Conversation
Reviewer's GuideThis PR replaces the custom cross-build matrix with a GoReleaser-driven GitHub Actions release job, adds a YAML inspection helper and accompanying Cucumber tests, updates docs for Linux-only packaging automation, and marks related roadmap tasks complete. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
Summary by CodeRabbit
WalkthroughStreamline the GitHub Actions release workflow by shifting from a custom multi-platform build matrix to a single GoReleaser-driven Linux job. Update documentation to reflect the Linux-only focus and deferred macOS support. Introduce a new Rust module and BDD-style tests to verify workflow configuration, including GoReleaser usage and tag-based triggers. Changes
Sequence Diagram(s)sequenceDiagram
participant Developer
participant GitHub Actions
participant GoReleaser
participant GitHub Release
Developer->>GitHub Actions: Push tag v*
GitHub Actions->>GoReleaser: Run GoReleaser job (Linux)
GoReleaser->>GitHub Release: Build, package, and upload assets
GoReleaser->>GitHub Release: Create draft release
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
General comments:
- You can reuse the
uses_goreleaserutility in your CucumberReleaseWorldsteps instead of duplicating YAML parsing logic. - Since
serde_yamlis only used for workflow inspection and tests, consider moving it to dev-dependencies rather than a normal dependency. - The workflow currently drops caching for Rust crates and build artifacts; reintroducing cache steps could significantly speed up subsequent runs.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You can reuse the `uses_goreleaser` utility in your Cucumber `ReleaseWorld` steps instead of duplicating YAML parsing logic.
- Since `serde_yaml` is only used for workflow inspection and tests, consider moving it to dev-dependencies rather than a normal dependency.
- The workflow currently drops caching for Rust crates and build artifacts; reintroducing cache steps could significantly speed up subsequent runs.
## Individual Comments
### Comment 1
<location> `.github/workflows/release.yml:7` </location>
<code_context>
on:
push:
tags:
- - 'v*.*.*'
-
-env:
- REPO_NAME: ${{ github.event.repository.name }}
+ - 'v*'
jobs:
</code_context>
<issue_to_address>
The tag pattern has been broadened and may match unintended tags.
This change will match any tag starting with 'v', not just semantic version tags. If you want to trigger only on semantic version tags, use a more specific pattern like 'v[0-9]+.[0-9]+.[0-9]+*'.
</issue_to_address>
### Comment 2
<location> `src/workflow.rs:10` </location>
<code_context>
+/// # Errors
+///
+/// Returns an error if the YAML cannot be parsed.
+pub fn uses_goreleaser(yaml: &str) -> Result<bool, serde_yaml::Error> {
+ let doc: Value = serde_yaml::from_str(yaml)?;
+ let Some(jobs) = doc.get("jobs") else {
+ return Ok(false);
+ };
+ let Some(goreleaser) = jobs.get("goreleaser") else {
+ return Ok(false);
+ };
+ let Some(steps) = goreleaser.get("steps") else {
+ return Ok(false);
+ };
+ let Some(arr) = steps.as_sequence() else {
+ return Ok(false);
+ };
+ for step in arr {
+ if let Some(uses) = step.get("uses")
+ && uses
+ .as_str()
+ .is_some_and(|s| s.contains("goreleaser-action"))
+ {
+ return Ok(true);
+ }
+ }
+ Ok(false)
+}
+
</code_context>
<issue_to_address>
The function only checks for the 'goreleaser' job and does not detect GoReleaser usage in other jobs.
This approach misses GoReleaser usage in jobs with different names. Please update the function to check all jobs for GoReleaser steps.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
let Some(jobs) = doc.get("jobs") else {
return Ok(false);
};
let Some(goreleaser) = jobs.get("goreleaser") else {
return Ok(false);
};
let Some(steps) = goreleaser.get("steps") else {
return Ok(false);
};
let Some(arr) = steps.as_sequence() else {
return Ok(false);
};
for step in arr {
if let Some(uses) = step.get("uses")
&& uses
.as_str()
.is_some_and(|s| s.contains("goreleaser-action"))
{
return Ok(true);
}
}
Ok(false)
}
=======
let Some(jobs) = doc.get("jobs") else {
return Ok(false);
};
let Some(jobs_map) = jobs.as_mapping() else {
return Ok(false);
};
for job in jobs_map.values() {
let Some(steps) = job.get("steps") else {
continue;
};
let Some(arr) = steps.as_sequence() else {
continue;
};
for step in arr {
if let Some(uses) = step.get("uses")
&& uses
.as_str()
.is_some_and(|s| s.contains("goreleaser-action"))
{
return Ok(true);
}
}
}
Ok(false)
}
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `src/workflow.rs:25` </location>
<code_context>
+ return Ok(false);
+ };
+ for step in arr {
+ if let Some(uses) = step.get("uses")
+ && uses
+ .as_str()
+ .is_some_and(|s| s.contains("goreleaser-action"))
+ {
+ return Ok(true);
</code_context>
<issue_to_address>
The check for 'goreleaser-action' may produce false positives if the string appears in a different context.
Using 'goreleaser-action' as a substring may match unrelated actions. Use a stricter check, such as verifying the prefix 'goreleaser/goreleaser-action'.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
.as_str()
.is_some_and(|s| s.contains("goreleaser-action"))
=======
.as_str()
.is_some_and(|s| s.starts_with("goreleaser/goreleaser-action"))
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `tests/features/release.feature:1` </location>
<code_context>
+Feature: Release workflow
+
+ Scenario: goreleaser step present
</code_context>
<issue_to_address>
Consider replacing the Cucumber feature with equivalent unit tests to simplify the test suite.
```markdown
Deleting this Cucumber feature and its glue code and leaning on your existing unit tests will keep us DRY and reduce maintenance overhead. You can cover the same two assertions in a tiny unit‐test file. For example, in JavaScript with Jest:
1. Remove:
- `features/release_workflow.feature`
- any related `features/step_definitions/*`
2. Add to your unit tests (e.g. `tests/releaseWorkflow.test.js`):
```js
const fs = require('fs')
const { parseWorkflow } = require('../src/workflowParser')
describe('Release workflow YAML', () => {
const yaml = fs.readFileSync('path/to/.github/workflows/release.yml', 'utf8')
const wf = parseWorkflow(yaml)
test('includes goreleaser step', () => {
const hasGoreleaser = wf.jobs.release.steps
.some(step => step.name === 'goreleaser')
expect(hasGoreleaser).toBe(true)
})
test('triggers on version tags', () => {
expect(wf.on.push.tags).toContain('v*.*.*')
})
})
```
This preserves all coverage without the extra BDD layer.
</issue_to_address>
### Comment 5
<location> `tests/steps/release_steps.rs:1` </location>
<code_context>
+//! Behavioural steps for the release workflow.
+#![expect(clippy::expect_used, reason = "simplify test failure output")]
+
</code_context>
<issue_to_address>
Consider replacing the Cucumber step-based tests with straightforward unit tests to simplify the test structure.
Here’s a way to keep the same checks but drop the Cucumber indirection and turn it into two simple unit-tests. You can remove the `cucumber` dependency and those step definitions, then add something like this in `tests/release_workflow.rs`:
```rust
use serde_yaml::Value;
use std::fs;
fn load_workflow() -> Value {
let text = fs::read_to_string(".github/workflows/release.yml")
.expect("failed to read workflow file");
serde_yaml::from_str(&text).expect("failed to parse workflow YAML")
}
#[test]
fn workflow_uses_goreleaser() {
let yaml = load_workflow();
let steps = yaml
.get("jobs")
.and_then(|j| j.get("goreleaser"))
.and_then(|g| g.get("steps"))
.and_then(Value::as_sequence)
.expect("steps must be a sequence");
assert!(
steps.iter().any(|s| {
s.get("uses")
.and_then(Value::as_str)
.map_or(false, |u| u.contains("goreleaser-action"))
}),
"expected a step using goreleaser-action"
);
}
#[test]
fn workflow_triggers_on_tags() {
let yaml = load_workflow();
let tags = yaml
.get("on")
.and_then(|o| o.get("push"))
.and_then(|p| p.get("tags"))
.and_then(Value::as_sequence)
.expect("tags must be a sequence");
assert!(
tags.iter().any(|t| t.as_str() == Some("v*")),
"expected push.tags to include \"v*\""
);
}
```
Steps to apply:
1. Remove the `cucumber`, `given/when/then` code and the `ReleaseWorld` struct.
2. Create a `tests/release_workflow.rs` file.
3. Paste the above helper and tests.
4. Run `cargo test` to verify the same assertions pass.
</issue_to_address>
### Comment 6
<location> `src/workflow.rs:38` </location>
<code_context>
+
+#[cfg(test)]
+mod tests {
+ #![expect(clippy::expect_used, reason = "simplify test output")]
+ use super::uses_goreleaser;
+
</code_context>
<issue_to_address>
Lint suppression uses a lint group (clippy::expect_used), which is not allowed per instructions.
The instruction forbids the use of lint groups for suppression. Please suppress only the specific lint, not a group, and ensure the scope is as narrow as possible.
</issue_to_address>
### Comment 7
<location> `docs/automated-cross-platform-packaging.md:7` </location>
<code_context>
-daemon for Linux (Fedora, Ubuntu) and macOS. We will use GoReleaser to manage
-the entire process, from building the Rust binaries to creating platform-native
-packages (`.rpm`, `.deb`) and a Homebrew formula.
+daemon for Linux (Fedora, Ubuntu) and macOS. Mac OS packaging is currently on
+hold, so the workflow focuses on Linux targets only. We will use GoReleaser to
+manage the entire process, from building the Rust binaries to creating
</code_context>
<issue_to_address>
The sentence uses the pronoun "we", which should be avoided per the instructions.
Consider rephrasing the sentence to avoid "we". For example: "GoReleaser is used to manage the entire process, from building the Rust binaries to creating platform-native packages..."
</issue_to_address>
### Comment 8
<location> `docs/automated-cross-platform-packaging.md:10` </location>
<code_context>
+daemon for Linux (Fedora, Ubuntu) and macOS. Mac OS packaging is currently on
+hold, so the workflow focuses on Linux targets only. We will use GoReleaser to
+manage the entire process, from building the Rust binaries to creating
+platform-native packages (`.rpm`, `.deb`) and a Homebrew formula.
The core of this process involves creating a `.goreleaser.yaml` file that
</code_context>
<issue_to_address>
This paragraph line exceeds 80 columns, violating the wrapping rule.
Please ensure all paragraph lines are wrapped to 80 columns.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (5)
tests/features/release.feature (1)
1-12: Reconsider the testing approach based on previous feedback.The Cucumber feature syntax is correct, but the previous review comment raises a valid point about simplifying the test suite. Unit tests would provide the same coverage with less maintenance overhead and complexity.
src/workflow.rs (2)
38-38: Fix lint suppression to use specific lint instead of group.The coding guidelines forbid lint groups. Replace
clippy::expect_usedwith the specific lint.- #![expect(clippy::expect_used, reason = "simplify test output")] + #![expect(clippy::missing_panics_doc, reason = "test module expects are acceptable")]
15-33: Broaden GoReleaser detection and strengthen string matching.The function currently only checks the "goreleaser" job, missing potential GoReleaser usage in other jobs. Additionally, the substring check for "goreleaser-action" may produce false positives.
Apply the suggested improvements from previous reviews to check all jobs and use stricter string matching:
- let Some(goreleaser) = jobs.get("goreleaser") else { - return Ok(false); - }; - let Some(steps) = goreleaser.get("steps") else { - return Ok(false); - }; - let Some(arr) = steps.as_sequence() else { - return Ok(false); - }; - for step in arr { - if let Some(uses) = step.get("uses") - && uses - .as_str() - .is_some_and(|s| s.contains("goreleaser-action")) - { - return Ok(true); - } + let Some(jobs_map) = jobs.as_mapping() else { + return Ok(false); + }; + for job in jobs_map.values() { + let Some(steps) = job.get("steps") else { + continue; + }; + let Some(arr) = steps.as_sequence() else { + continue; + }; + for step in arr { + if let Some(uses) = step.get("uses") + && uses + .as_str() + .is_some_and(|s| s.starts_with("goreleaser/goreleaser-action")) + { + return Ok(true); + } + } }tests/steps/release_steps.rs (1)
1-55: Well-implemented but reconsider the testing approach.The code quality is good with proper doc comments and error handling. However, the previous review comment raises valid concerns about complexity and maintenance overhead. The logic here duplicates functionality from
src/workflow.rsand could be simplified with direct unit tests.The implementation itself is solid, but evaluate whether the BDD approach provides sufficient value over simpler unit tests for this use case.
.github/workflows/release.yml (1)
5-6: Tighten tag pattern to semantic-version tags only.The broadened pattern
'v*'now fires for any tag that merely starts withv, e.g.v-nextorvacation-photo. Restrict the trigger to semantic-version tags to avoid accidental releases.- - 'v*' + - 'v[0-9]+.[0-9]+.[0-9]+*'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/release.yml(1 hunks)Cargo.toml(1 hunks)docs/automated-cross-platform-packaging.md(2 hunks)docs/comenq-design.md(1 hunks)docs/roadmap.md(1 hunks)src/lib.rs(1 hunks)src/workflow.rs(1 hunks)tests/cucumber.rs(1 hunks)tests/features/release.feature(1 hunks)tests/steps/mod.rs(1 hunks)tests/steps/release_steps.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/roadmap.mddocs/comenq-design.mddocs/automated-cross-platform-packaging.md
**/*.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
src/lib.rstests/steps/mod.rstests/cucumber.rssrc/workflow.rstests/steps/release_steps.rs
🧬 Code Graph Analysis (2)
src/workflow.rs (1)
tests/steps/release_steps.rs (1)
uses_goreleaser(27-41)
tests/steps/release_steps.rs (1)
src/workflow.rs (1)
uses_goreleaser(10-34)
🪛 LanguageTool
docs/comenq-design.md
[uncategorized] ~726-~726: Use a comma before ‘so’ if it connects two independent clauses (unless they are closely connected and short).
Context: ...lease. Mac support is currently deferred so the workflow targets Linux only. ## Se...
(COMMA_COMPOUND_SENTENCE_2)
docs/automated-cross-platform-packaging.md
[grammar] ~7-~7: The operating system from Apple is written “macOS”
Context: ...n for Linux (Fedora, Ubuntu) and macOS. Mac OS packaging is currently on hold, so the ...
(MAC_OS)
🔇 Additional comments (3)
src/lib.rs (1)
6-8: Remove outdated docs suggestionThe
src/workflow.rsmodule already opens with://! Utilities for inspecting GitHub workflow files.No further action required.
tests/cucumber.rs (1)
5-6: LGTM! Clean integration of ReleaseWorld.The addition follows the established pattern for test world integration and maintains consistency with existing imports and concurrent execution structure.
Also applies to: 13-13
docs/roadmap.md (1)
108-119: LGTM! Milestone completion properly documented.The roadmap updates accurately reflect the completion of the release workflow automation tasks, providing clear tracking of project progress.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/steps/release_steps.rs (1)
2-2: Replace lint group suppression with specific lint.Same issue as in
workflow.rs- use specific lint instead of group:-#![expect(clippy::expect_used, reason = "simplify test failure output")] +#![expect(clippy::unwrap_used, reason = "simplify test failure output")]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/release.yml(1 hunks)docs/automated-cross-platform-packaging.md(3 hunks)docs/comenq-design.md(1 hunks)src/workflow.rs(1 hunks)tests/steps/mod.rs(2 hunks)tests/steps/release_steps.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
⚙️ CodeRabbit Configuration File
**/*.rs: * Seek to keep the cyclomatic complexity of functions no more than 12.
Adhere to single responsibility and CQRS
Place function attributes after doc comments.
Do not use
returnin single-line functions.Move conditionals with >2 branches into a predicate function.
Avoid
unsafeunless absolutely necessary.Every module must begin with a
//!doc comment that explains the module's purpose and utility.Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar
Lints must not be silenced except as a last resort.
#[allow]is forbidden.- Only narrowly scoped
#[expect(lint, reason = "...")]is allowed.- No lint groups, no blanket or file-wide suppression.
- Include
FIXME:with link if a fix is expected.Use
rstestfixtures for shared setup and to avoid repetition between tests.Replace duplicated tests with
#[rstest(...)]parameterised cases.Prefer
mockallfor mocks/stubs.Prefer
.expect()over.unwrap()Ensure that any API or behavioural changes are reflected in the documentation in
docs/Ensure that any completed roadmap steps are recorded in the appropriate roadmap in
docs/Files must not exceed 400 lines in length
- Large modules must be decomposed
- Long match statements or dispatch tables should be decomposed by domain and collocated with targets
- Large blocks of inline data (e.g., test fixtures, constants or templates) must be moved to external files and inlined at compile-time or loaded at run-time.
Files:
tests/steps/mod.rstests/steps/release_steps.rssrc/workflow.rs
**/*.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
docs/comenq-design.mddocs/automated-cross-platform-packaging.md
🪛 actionlint (1.7.7)
.github/workflows/release.yml
21-21: input "cache" is not defined in action "dtolnay/rust-toolchain@stable". available inputs are "components", "target", "targets", "toolchain"
(action)
🔇 Additional comments (8)
docs/automated-cross-platform-packaging.md (2)
281-282: LGTM! Linux-only targets align with PR objectives.The removal of macOS targets and addition of Cargo caching properly reflects the workflow's focus on Linux packaging only.
298-298: Good practice pinning GoReleaser version.Pinning to
v1.24.0instead oflatestimproves build reproducibility and prevents unexpected breaking changes.docs/comenq-design.md (1)
724-726: Excellent documentation of the release workflow.The addition clearly explains the automated release process using GitHub Actions and GoReleaser, properly documenting the Linux-focused approach.
tests/steps/mod.rs (1)
13-14: Proper integration of release workflow tests.Adding the
release_stepsmodule andReleaseWorldexport enables BDD-style testing of the release workflow configuration.src/workflow.rs (2)
10-36: Well-structured workflow inspection utility.The function properly handles YAML parsing, iterates through all jobs and steps, and uses strict prefix matching for goreleaser detection. Error handling is appropriate.
40-40: Replace lint group suppression with specific lint.The coding guidelines forbid lint group suppression. Replace with the specific lint:
- #![expect(clippy::expect_used, reason = "simplify test output")] + #![expect(clippy::unwrap_used, reason = "simplify test output")]Likely an incorrect or invalid review comment.
tests/steps/release_steps.rs (1)
15-47: Comprehensive BDD testing for release workflow.The step definitions properly validate workflow configuration, including goreleaser usage detection and tag trigger patterns. The implementation follows established testing patterns.
.github/workflows/release.yml (1)
17-22: Fix invalidcacheinput fordtolnay/rust-toolchain.
cache: cargois not a recognised input (flagged by actionlint). The action expects eithercache: true/falseor the list-of-paths syntax. Use the boolean to enable the built-in cargo cache.- cache: cargo + # Enable the built-in cargo cache + cache: trueLikely an incorrect or invalid review comment.
Summary
Testing
make lintmake testmake markdownlintmake nixiehttps://chatgpt.com/codex/tasks/task_e_688bf7894f1c8322af2fc4b7d630ee24
Summary by Sourcery
Automate and streamline the release process by integrating GoReleaser into the GitHub Actions workflow and updating related code, documentation, and tests.
New Features:
Enhancements:
CI:
Documentation:
Tests: