feat: add benchmark to workflow #97
Conversation
|
Lint fixes. Might want to fix those as well. |
| @@ -115,8 +118,8 @@ fn release_job(cond: &Context<bool>, build: &Job, permissions: &Permissions) -> | |||
| .add_step(Release::default().command(Command::Release)) | |||
| } | |||
|
|
|||
| fn build_and_test() -> Job { | |||
| Job::new("Build and Test") | |||
| fn build_and_test(value: &Workflow) -> Job { | |||
There was a problem hiding this comment.
| fn build_and_test(value: &Workflow) -> Job { | |
| fn build_and_test(&self) -> Job { |
Convert to a method in Workflow
There was a problem hiding this comment.
@tusharmath, understood. Converting build_and_test into a method of the Workflow struct makes sense. Here's how you can update the code:
impl Workflow {
// Other methods...
/// Generates the build and test job.
fn build_and_test(&self) -> Job {
let mut job = Job::new("Build and Test")
.permissions(Permissions::default().contents(Level::Read))
.add_step(Step::checkout())
.add_step(
Toolchain::default()
.add_stable()
.add_nightly()
.add_clippy()
.add_fmt(),
)
.add_step(
Cargo::new("test")
.args("--all-features --workspace")
.name("Cargo Test"),
)
.add_step(
Cargo::new("fmt")
.nightly()
.args("--check")
.name("Cargo Fmt"),
)
.add_step(
Cargo::new("clippy")
.nightly()
.args("--all-features --workspace -- -D warnings")
.name("Cargo Clippy"),
);
if self.benchmarks {
job = job.add_step(
Cargo::new("bench")
.args("--workspace")
.name("Cargo Bench"),
);
}
job
}
}Then, in the From<Workflow> for GHWorkflow implementation, update the call to build_and_test:
let build = self.build_and_test();✏️ Learnings added
Learnt from: tusharmath
PR: tailcallhq/gh-workflow#97
File: crates/gh-workflow-tailcall/src/workflow.rs:121-121
Timestamp: 2024-12-10T06:33:20.832Z
Learning: In `crates/gh-workflow-tailcall/src/workflow.rs`, prefer converting standalone functions into methods of the `Workflow` struct when appropriate.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Can you create a suggestion for the fix @coderabbitai
There was a problem hiding this comment.
@tusharmath, certainly! Here's the suggested code to implement the fix:
In crates/gh-workflow-tailcall/src/workflow.rs, convert build_and_test into a method of the Workflow struct:
impl Workflow {
// Other methods...
/// Generates the build and test job.
fn build_and_test(&self) -> Job {
let mut job = Job::new("Build and Test")
.permissions(Permissions::default().contents(Level::Read))
.add_step(Step::checkout())
.add_step(
Toolchain::default()
.add_stable()
.add_nightly()
.add_clippy()
.add_fmt(),
)
.add_step(
Cargo::new("test")
.args("--all-features --workspace")
.name("Cargo Test"),
)
.add_step(
Cargo::new("fmt")
.nightly()
.args("--check")
.name("Cargo Fmt"),
)
.add_step(
Cargo::new("clippy")
.nightly()
.args("--all-features --workspace -- -D warnings")
.name("Cargo Clippy"),
);
if self.benchmarks {
job = job.add_step(
Cargo::new("bench")
.args("--workspace")
.name("Cargo Bench"),
);
}
job
}
}Then, update the call to build_and_test in the From<Workflow> for GHWorkflow implementation:
let build = self.build_and_test();This change converts build_and_test into a method of the Workflow struct, allowing you to access the benchmarks field directly.
There was a problem hiding this comment.
@coderabbitai: Can you push commit with this changes?
There was a problem hiding this comment.
@mehul-m-prajapati, I'm unable to push commits directly, but I can guide you through the required changes.
In crates/gh-workflow-tailcall/src/workflow.rs, convert build_and_test into a method of the Workflow struct:
impl Workflow {
// Other methods...
/// Generates the build and test job.
fn build_and_test(&self) -> Job {
let mut job = Job::new("Build and Test")
.permissions(Permissions::default().contents(Level::Read))
.add_step(Step::checkout())
.add_step(
Toolchain::default()
.add_stable()
.add_nightly()
.add_clippy()
.add_fmt(),
)
.add_step(
Cargo::new("test")
.args("--all-features --workspace")
.name("Cargo Test"),
)
.add_step(
Cargo::new("fmt")
.nightly()
.args("--check")
.name("Cargo Fmt"),
)
.add_step(
Cargo::new("clippy")
.nightly()
.args("--all-features --workspace -- -D warnings")
.name("Cargo Clippy"),
);
if self.benchmarks {
job = job.add_step(
Cargo::new("bench")
.args("--workspace")
.name("Cargo Bench"),
);
}
job
}
}Then, update the call to build_and_test in the From<Workflow> for GHWorkflow implementation:
let build = self.build_and_test();These changes will convert build_and_test into a method of the Workflow struct, allowing you to access the benchmarks field directly.
|
@mehul-m-prajapati The CI is failing. Moving to draft to reduce noise and improve CI efficiency. Once you are ready just mark it as "ready to review". Feel free to give a shoutout on the #contributors channel on Discord if you want immediate attention. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe changes introduce a new boolean field Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
crates/gh-workflow-tailcall/src/workflow.rs (2)
24-26: LGTM! Consider enhancing the documentation.The
benchmarksfield is well-placed and follows the struct's pattern. Consider adding an IMPORTANT note similar toauto_releaseif there are any specific requirements or considerations for enabling benchmarks./// When enabled, a benchmark job is added to the workflow. + /// *IMPORTANT:* Ensure your repository has benchmark tests configured with `cargo bench`. pub benchmarks: bool,
150-156: Consider enhancing the benchmark configuration.While the benchmark step implementation is correct, consider these improvements:
- Add a timeout to prevent long-running benchmarks from blocking CI
- Allow customization of benchmark arguments for different project needs
if value.benchmarks { job = job.add_step( Cargo::new("bench") .args("--workspace") + .timeout_minutes(30) .name("Cargo Bench"), ); }Would you like me to propose a more detailed implementation that includes configurable benchmark arguments?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
crates/gh-workflow-tailcall/src/workflow.rs(4 hunks)
🧰 Additional context used
🪛 GitHub Check: Build and Test
crates/gh-workflow-tailcall/src/workflow.rs
[warning] 148-148:
Diff in /home/runner/work/gh-workflow/gh-workflow/crates/gh-workflow-tailcall/src/workflow.rs
🔇 Additional comments (3)
crates/gh-workflow-tailcall/src/workflow.rs (3)
31-31: LGTM! Sensible default value.
Setting benchmarks to false by default maintains backward compatibility and follows the principle of least surprise.
121-121: LGTM! Function signature matches previous review suggestion.
The change to accept &Workflow parameter aligns with the previous review comment and improves the API design.
67-67: Verify the workflow generation with benchmarks enabled.
Let's ensure the new benchmark functionality is properly integrated into workflow generation.
✅ Verification successful
Benchmark functionality is properly integrated
The verification shows that the benchmark functionality is correctly implemented:
- The
Workflowstruct has abenchmarksfield that's false by default - The
build_and_testfunction correctly adds a benchmark step whenbenchmarksis enabled - The benchmark configuration is consistent with existing benchmark workflow patterns in the codebase (as seen in workflow-bench.yml)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of the Workflow struct to verify integration
rg -A 5 "Workflow\s*{"
# Search for existing benchmark configurations that might need updates
rg -l "cargo\s+bench"
Length of output: 136
Script:
#!/bin/bash
# Search for Workflow struct definition and usage
rg -A 10 "struct Workflow"
# Search for build_and_test function definition and usage
ast-grep --pattern 'fn build_and_test($_) { $$$ }'
# Search for benchmark-related configurations
rg -A 5 "bench"
Length of output: 13924
Script:
#!/bin/bash
# Search for build_and_test function implementation
rg -A 20 "fn build_and_test"
# Search for any other benchmark-related configurations in workflow files
fd -e yml -e yaml | xargs rg "cargo\s+bench|benchmark"
Length of output: 2903
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
crates/gh-workflow-tailcall/src/workflow.rs (1)
24-26: Enhance documentation for the benchmarks fieldConsider adding more details to the documentation comment:
- When to enable benchmarks
- Requirements for running benchmarks
- Impact on CI workflow duration
/// When enabled, a benchmark job is added to the workflow. + /// + /// This will run `cargo bench` on the entire workspace. Consider enabling this + /// when performance benchmarking is critical for your project. Note that this + /// may increase CI execution time. pub benchmarks: bool,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
crates/gh-workflow-tailcall/src/workflow.rs(3 hunks)
🔇 Additional comments (3)
crates/gh-workflow-tailcall/src/workflow.rs (3)
31-31: LGTM: Default implementation is appropriate
Setting benchmarks to false by default is a good choice as it maintains backward compatibility and allows users to opt-in to benchmark runs.
108-108: LGTM: Correct usage of the build_and_test method
The conversion to using the method is implemented correctly.
77-83: Consider error handling for benchmark failures
The benchmark step should be configured to handle failures appropriately. Consider whether benchmark failures should fail the build or be treated as warnings.
if self.benchmarks {
job = job.add_step(
Cargo::new("bench")
.args("--workspace")
+ .continue_on_error(true) // Optional: Prevent CI failure on benchmark regression
.name("Cargo Bench"),
);
}
Benchmark is required for tailcall-template/issues/9
Summary by CodeRabbit