Move serde_yaml to dev deps and streamline packaging#43
Conversation
Reviewer's GuideThis PR adds a guard to the Linux preremove script for environments without systemd, centralises and annotates the default config template, shifts serde_yaml to dev-dependencies while moving workflow helpers into the test-support crate, and updates documentation and tests to reflect these changes. Class diagram for serde_yaml and workflow module refactorclassDiagram
class MainCrate {
+serde
+serde_json
-serde_yaml (removed from main dependencies)
-workflow (module removed)
}
class TestSupport {
+serde_yaml (added as dev-dependency)
+workflow (helpers moved here)
}
MainCrate <.. TestSupport : uses (in tests)
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 10 minutes and 45 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 ignored due to path filters (1)
📒 Files selected for processing (4)
Summary by CodeRabbit
WalkthroughUpdate packaging and configuration file paths for Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Suite
participant TestSupport as test-support::workflow
Test->>TestSupport: use uses_goreleaser()
TestSupport-->>Test: Provide uses_goreleaser functionality
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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 (
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes - here's some feedback:
- The docs mention using the goreleaser-rust plugin but your .goreleaser.yaml doesn’t include a sample plugin configuration—consider adding a snippet or updating the config to demonstrate how to integrate it.
- The release_steps test now uses the wildcard matcher
v*.*.*, which won’t enforce numeric segments; consider tightening it (e.g. with a regex) to avoid matching unintended tags. - Double-check any scripts or CI steps for lingering references to the old
packaging/comenqdpath and update them to the newpackaging/config/comenqd.tomlto prevent packaging errors.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The docs mention using the goreleaser-rust plugin but your .goreleaser.yaml doesn’t include a sample plugin configuration—consider adding a snippet or updating the config to demonstrate how to integrate it.
- The release_steps test now uses the wildcard matcher `v*.*.*`, which won’t enforce numeric segments; consider tightening it (e.g. with a regex) to avoid matching unintended tags.
- Double-check any scripts or CI steps for lingering references to the old `packaging/comenqd` path and update them to the new `packaging/config/comenqd.toml` to prevent packaging errors.
## Individual Comments
### Comment 1
<location> `tests/steps/release_steps.rs` </location>
<code_context>
- assert!(
- tags.iter()
- .any(|t| t.as_str() == Some("v[0-9]*.[0-9]*.[0-9]*"))
- );
+ assert!(tags.iter().any(|t| t.as_str() == Some("v*.*.*")));
</code_context>
<issue_to_address>
Test assertion pattern change may reduce strictness.
The new pattern is less strict and may allow unintended tag formats. Confirm if this is intentional; otherwise, update the test to enforce the correct versioning scheme.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| .expect("tags") | ||
| .as_sequence() | ||
| .expect("sequence"); | ||
| assert!( |
There was a problem hiding this comment.
issue (testing): Test assertion pattern change may reduce strictness.
The new pattern is less strict and may allow unintended tag formats. Confirm if this is intentional; otherwise, update the test to enforce the correct versioning scheme.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
tests/steps/release_steps.rs (1)
43-43: Relaxed tag-matching remains a known risk.
The patternv*.*.*allows any tag beginning withv, potentially matching invalid formats (e.g.vfoo). The stricter numeric glob or a regex would safeguard semantic-version tags.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.goreleaser.yaml(2 hunks)Cargo.toml(1 hunks)crates/comenq/Cargo.toml(1 hunks)crates/comenq/src/client.rs(0 hunks)docs/automated-cross-platform-packaging.md(6 hunks)packaging/config/comenqd.toml(1 hunks)packaging/linux/preremove.sh(1 hunks)src/lib.rs(0 hunks)test-support/Cargo.toml(1 hunks)test-support/src/lib.rs(1 hunks)tests/steps/release_steps.rs(2 hunks)
💤 Files with no reviewable changes (2)
- crates/comenq/src/client.rs
- src/lib.rs
🧰 Additional context used
📓 Path-based instructions (5)
Cargo.toml
📄 CodeRabbit Inference Engine (AGENTS.md)
Cargo.toml: Use explicit version ranges in Cargo.toml and keep dependencies up-to-date.
Mandate caret requirements for all dependencies. All crate versions specified in Cargo.toml must use SemVer-compatible caret requirements (e.g., some-crate = "1.2.3").
Prohibit unstable version specifiers. The use of wildcard (*) or open-ended inequality (>=) version requirements are strictly forbidden, as are tilde requirements (~) except for documented reasons.
Files:
Cargo.toml
**/*.rs
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.rs: Clippy warnings MUST be disallowed.
Fix any warnings emitted during tests in the code itself rather than silencing them.
Where a function is too long, extract meaningfully named helper functions adhering to separation of concerns and CQRS.
Where a function has too many parameters, group related parameters in meaningfully named structs.
Where a function is returning a large error consider using Arc to reduce the amount of data returned.
Write unit and behavioural tests for new functionality. Run both before and after making any change.
Every module must begin with a module level (//! ) comment explaining the module's purpose and utility.
Document public APIs using Rustdoc comments (///) so documentation can be generated with cargo doc.
Prefer immutable data and avoid unnecessary mut bindings.
Handle errors with the Result type instead of panicking where feasible.
Avoid unsafe code unless absolutely necessary and document any usage clearly.
Place function attributes after doc comments.
Do not use return in single-line functions.
Use predicate functions for conditional criteria with more than two branches.
Lints must not be silenced except as a last resort.
Lint rule suppressions must be tightly scoped and include a clear reason.
Prefer expect over allow.
Use rstest fixtures for shared setup.
Replace duplicated tests with #[rstest(...)] parameterised cases.
Prefer mockall for mocks/stubs.
Prefer .expect() over .unwrap().
Use concat!() to combine long string literals rather than escaping newlines with a backslash.
Prefer semantic error enums. Derive std::error::Error (via the thiserror crate) for any condition the caller might inspect, retry, or map to an HTTP status.
Use an opaque error only at the app boundary. Use eyre::Report for human-readable logs; these should not be exposed in public APIs.
Never export the opaque type from a library. Convert to domain enums at API boundaries, and to eyre only in the main main() entrypoint or top-level async task.
Files:
tests/steps/release_steps.rstest-support/src/lib.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/release_steps.rstest-support/src/lib.rs
docs/**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
docs/**/*.md: Use the markdown files within the docs/ directory as a knowledge base and source of truth for project requirements, dependency choices, and architectural decisions.
When new decisions are made, requirements change, libraries are added/removed, or architectural patterns evolve, proactively update the relevant file(s) in the docs/ directory to reflect the latest state. Ensure the documentation remains accurate and current.
Files:
docs/automated-cross-platform-packaging.md
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
**/*.md: Documentation must use en-GB-oxendict spelling and grammar. (EXCEPTION: the naming of the LICENSE file, which is to be left unchanged for community consistency.)
Validate Markdown files using make markdownlint.
Run make fmt after any documentation changes to format all Markdown files and fix table markup.
Validate Mermaid diagrams in Markdown files by running make nixie.
Markdown paragraphs and bullet points must be wrapped at 80 columns.
Code blocks must be wrapped at 120 columns.
Tables and headings must not be wrapped.
Use dashes (-) for list bullets.
Use GitHub-flavoured Markdown footnotes ([^1]) for references and footnotes.
Files:
docs/automated-cross-platform-packaging.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/automated-cross-platform-packaging.md
docs/**/*
📄 CodeRabbit Inference Engine (docs/roadmap.md)
Add a new document to the
/docsdirectory detailing the automated packaging process for future maintainers and contributors.
Files:
docs/automated-cross-platform-packaging.md
🧬 Code Graph Analysis (2)
tests/steps/release_steps.rs (1)
test-support/src/workflow.rs (1)
uses_goreleaser(10-36)
test-support/src/lib.rs (1)
test-support/src/workflow.rs (1)
uses_goreleaser(10-36)
🔇 Additional comments (5)
test-support/Cargo.toml (1)
12-12: LGTM – workspace-scopedserde_yamladdition is correct.packaging/config/comenqd.toml (1)
3-4: LGTM – documentation clarifies behaviour when token is empty.Cargo.toml (1)
25-25: LGTM – move ofserde_yamlto dev-deps aligns with build goals.tests/steps/release_steps.rs (1)
7-7: Import update aligns tests with refactor – good.
The switch totest_support::uses_goreleasercleanly decouples the test suite from production code and keeps helpers in the dedicated crate..goreleaser.yaml (1)
32-32: Confirm new config path presence
Verified thatpackaging/config/comenqd.tomlexists in the repository. No further action required.
| [dev-dependencies] | ||
| rstest = "0.18" | ||
| tempfile = { workspace = true } |
There was a problem hiding this comment.
Specify full SemVer for rstest.
The guideline mandates explicit caret requirements with a full version (MAJOR.MINOR.PATCH). The current "0.18" omits the patch component.
-rstest = "0.18"
+rstest = "0.18.0"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [dev-dependencies] | |
| rstest = "0.18" | |
| tempfile = { workspace = true } | |
| [dev-dependencies] | |
| rstest = "0.18.0" | |
| tempfile = { workspace = true } |
🤖 Prompt for AI Agents
In crates/comenq/Cargo.toml around lines 18 to 20, the version for the rstest
dependency is specified as "0.18" which lacks the patch version. Update the
version string to a full SemVer format including major, minor, and patch
numbers, for example "0.18.0", to comply with the explicit caret requirement
guideline.
| > Note: GoReleaser provides a `goreleaser-rust` plugin and Docker-based builds | ||
| > that remove the need for the Go builder and manual Cargo hooks. Evaluating | ||
| > these approaches may streamline cross-compilation. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Wrap long prose to 80 columns.
Lines exceed the mandated 80-character limit for Markdown text, which will fail markdownlint (MD013). Re-wrap the quoted note accordingly.
🤖 Prompt for AI Agents
In docs/automated-cross-platform-packaging.md around lines 132 to 135, the note
about GoReleaser exceeds the 80-character line length limit required by
markdownlint (MD013). Reformat the text by inserting line breaks so that no line
exceeds 80 characters, ensuring the prose remains clear and readable while
complying with the style guide.
| if command -v systemctl >/dev/null && [ -d /run/systemd/system ]; then | ||
| if systemctl is-active --quiet comenqd.service; then | ||
| systemctl stop comenqd.service | ||
| fi | ||
| if systemctl is-enabled --quiet comenqd.service; then | ||
| systemctl disable comenqd.service | ||
| fi |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Refresh systemd state after disabling the service.
Invoke systemctl daemon-reload once the unit is disabled to purge stale state files during package removal.
if systemctl is-enabled --quiet comenqd.service; then
systemctl disable comenqd.service
+ systemctl daemon-reload
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if command -v systemctl >/dev/null && [ -d /run/systemd/system ]; then | |
| if systemctl is-active --quiet comenqd.service; then | |
| systemctl stop comenqd.service | |
| fi | |
| if systemctl is-enabled --quiet comenqd.service; then | |
| systemctl disable comenqd.service | |
| fi | |
| if command -v systemctl >/dev/null && [ -d /run/systemd/system ]; then | |
| if systemctl is-active --quiet comenqd.service; then | |
| systemctl stop comenqd.service | |
| fi | |
| if systemctl is-enabled --quiet comenqd.service; then | |
| systemctl disable comenqd.service | |
| systemctl daemon-reload | |
| fi |
🤖 Prompt for AI Agents
In packaging/linux/preremove.sh around lines 4 to 10, after disabling the
comenqd.service with systemctl disable, add a call to systemctl daemon-reload to
refresh the systemd manager configuration and remove stale state files. This
ensures systemd properly updates its state during package removal.
| pub mod workflow; | ||
|
|
||
| pub use daemon::{octocrab_for, temp_config}; | ||
| pub use util::wait_for_file; | ||
| pub use workflow::uses_goreleaser; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add module-level docs for the new workflow helper.
test-support/src/workflow.rs appears without a //! header and its public API lacks Rustdoc. Add concise module docs and function docs to satisfy the repository guidelines.
🤖 Prompt for AI Agents
In test-support/src/lib.rs around lines 5 to 9, the new workflow module is
publicly used but lacks module-level documentation. Add a concise //! doc
comment at the top of test-support/src/workflow.rs describing the module's
purpose, and add Rustdoc comments to its public functions to meet repository
documentation standards.
Switch to path-based inclusion of shared env helpers, derive Clone for Config and replace test utility dependencies with local helpers to resolve build failures.
|
Stale |
1 participant
Summary
Testing
make fmtmake lintmake testmake markdownlinthttps://chatgpt.com/codex/tasks/task_e_688f6f4f59908322a7b76b8fcd54248d
Summary by Sourcery
Refactor packaging layouts and dependencies, update scripts, and streamline tests
Bug Fixes:
Enhancements:
Build:
Tests: