Consolidate provider crate versions via workspace inheritance - #256
Consolidate provider crate versions via workspace inheritance#256reyhankoyun wants to merge 3 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #256 +/- ##
==========================================
- Coverage 86.35% 86.11% -0.24%
==========================================
Files 34 34
Lines 9422 9422
Branches 9422 9422
==========================================
- Hits 8136 8114 -22
- Misses 1153 1175 +22
Partials 133 133 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Claude finished @reyhankoyun's task in 2m 12s —— View job PR Review — Consolidate provider crate versions via workspace inheritance
Verdict: 👍 Solid, low-risk change. The workspace inheritance is set up correctly and the reasoning is sound. A couple of observations below, none blocking. What's correct
Observations (non-blocking)1. Residual version duplication in the root 2. aws_certificatemanager_provider = { path = "../aws_certificatemanager_provider" }Since 3. Removed blank line after Security / performanceNo concerns. This is metadata-only; no code paths, dependencies, or feature flags changed. Codecov delta is unrelated noise (file-count shift), consistent with no logic change. Testing noteYour reasoning that the compilation failures are due to local rustc Nice cleanup — this meaningfully reduces release footguns even with the residual duplication noted above. |
reyhankoyun
added
the
safe-to-test
github-actions
Bot
removed
the
safe-to-test
![secrets-manager-ai-code-review[bot]](https://avatars.githubusercontent.com/in/4081740?s=60&v=4){kind=small}
| aws_workload_credentials_provider_common = { version = "3.1.1", path = "aws_workload_credentials_provider_common" } | ||
| aws_secretsmanager_provider = { version = "3.1.1", path = "aws_secretsmanager_provider" } | ||
| aws_certificatemanager_provider = { version = "3.1.1", path = "aws_certificatemanager_provider" } |
There was a problem hiding this comment.
Residual duplication / version-mismatch risk. The three internal provider crates share the same version as [workspace.package] (3.1.1), but here their version is hardcoded again. A future bump now touches 4 places in this one file — workspace.package.version plus these three — instead of the intended single source of truth.
More importantly, if someone bumps workspace.package.version to e.g. 3.2.0 but forgets these entries, cargo publish will emit a manifest where (say) aws_secretsmanager_provider@3.2.0 depends on aws_workload_credentials_provider_common = "3.1.1" — the exact version-skew class of bug this PR aims to eliminate, just relocated.
Stable Cargo can't reference workspace.package.version from workspace.dependencies, so full deduplication isn't possible here. Two mitigations worth considering:
- Keep all four in this file (as done) and rely on release tooling (
cargo-release/cargo-workspaces) to bump them atomically, or - Add a brief comment noting these three must track
workspace.package.version.
Not blocking — this is still a net improvement over four separate files.
|
Claude finished @reyhankoyun's task in 4m 9s —— View job PR Review — Consolidate provider crate versions via workspace inheritanceVerdict: 👍 Solid, low-risk change, and this revision addresses the version-duplication concern from the earlier review by adding
What's correct
Response to the prior review's duplication concernThe earlier review flagged that
Minor / optional
Security / performanceNo concerns — metadata-only change. No code paths, runtime deps, or feature flags altered. Codecov's file-count delta is unrelated noise. Testing noteThe rustc Nice iteration — the |
| tag-message = "chore: release {{version}}" | ||
| tag-name = "v{{version}}" | ||
| # Publishing is handled by the publish-crates.yml GitHub Actions workflow. | ||
| publish = false |
There was a problem hiding this comment.
This release.toml is the right answer to the version-duplication concern raised in the earlier review — cargo-release can bump [workspace.package].version and rewrite the version fields in [workspace.dependencies] atomically, so the four providers no longer skew apart on a release.
Two things worth confirming before relying on it:
-
Verify the atomic bump with a dry run. Please run
cargo release <next-version> --dry-runand confirm the generated diff updates (a)workspace.package.version, (b) all three providerversionentries in[workspace.dependencies], and (c) leavesaws_secretsmanager_cachingat its independent2.1.0. That dry-run output is the real proof this eliminates the skew risk. Since the four providers inherit viaversion.workspace = true, you may needshared-version = truehere forcargo-releaseto treat them as one version group — the dry run will tell you. -
publish = false+ missing workflow. The comment on line 6 says publishing is handled bypublish-crates.yml, but I don't see that workflow in.github/workflows/(onlyrelease.yml/staging.yml, which upload binaries to S3 — not crates.io). If crates.io publishing is intended, that workflow still needs to land; otherwise the comment is misleading. Not blocking this PR, just flagging the dangling reference.
Reduces the number of files that need updating during a version bump from 4 separate Cargo.toml files to a single root Cargo.toml.
e9a9440 to
3adfdea
Compare
|
Claude finished @reyhankoyun's task in 2m 54s —— View job PR Review — Consolidate provider crate versions via workspace inheritance
Verdict: 👍 Solid, low-risk, metadata-only change. Workspace inheritance is wired correctly and this revision closes the biggest gap from the earlier reviews — What's correct
Observations (non-blocking)1. 2. Dry-run artifact + Security / performanceNo concerns — metadata-only. No code paths, runtime deps, or feature flags changed. Codecov's file-count delta is unrelated noise. Nice iteration. The version-skew risk is now managed by tooling rather than operator discipline, and the missing publish workflow that earlier reviews flagged has landed. Adding |
| tag-message = "chore: release {{version}}" | ||
| tag-name = "v{{version}}" | ||
| # Publishing is handled by the publish-crates.yml GitHub Actions workflow. | ||
| publish = false |
There was a problem hiding this comment.
shared-version grouping is implied but never enabled here. aws_secretsmanager_caching/release.toml explicitly opts out with shared-version = false — which only makes sense if the four provider crates are opted in to a shared group. But this root release.toml never sets shared-version = true, and cargo-release's default is false.
Practically the four providers can't diverge (they all resolve to the single [workspace.package].version via version.workspace = true), but without shared-version = true cargo-release treats each package as its own release unit rather than one atomic group, and aws_secretsmanager_caching's shared-version = false is a no-op override of a default that's already false.
I'd add it explicitly so the intent is encoded and the caching opt-out is meaningful:
| publish = false | |
| allow-branch = ["main"] | |
| consolidate-commits = true | |
| shared-version = true | |
| pre-release-commit-message = "chore: release {{version}}" | |
| tag-message = "chore: release {{version}}" | |
| tag-name = "v{{version}}" | |
| # Publishing is handled by the publish-crates.yml GitHub Actions workflow. | |
| publish = false |
The cargo release <ver> --dry-run diff (requested in the prior review) is the artifact that confirms which behavior you actually get.
| @@ -0,0 +1,7 @@ | |||
| allow-branch = ["main"] | |||
There was a problem hiding this comment.
Note that allow-branch = ["main"] means the dry-run shown in the PR description (which ran on consolidate-workspace-version and printed Pushing consolidate-workspace-version, v3.1.2 to origin) would now be refused on this feature branch. That's the correct safety posture for real releases — just flagging that the description's dry-run output predates this constraint, so a fresh dry-run (post-merge or with --allow-branch) is what actually validates the config as it stands.
Configures cargo-release so that bumping the provider version is a single command (e.g. `cargo release patch --execute`) that atomically updates workspace.package.version and all workspace.dependencies entries, commits, tags, and pushes. Publishing is disabled here since it is handled by the publish-crates.yml workflow.
3adfdea to
0dca2c5
Compare
PR Review — Consolidate provider crate versions via workspace inheritance
|
reyhankoyun
added
the
safe-to-test
|
Claude finished @reyhankoyun's task in 1m 24s —— View job PR Review — Consolidate provider crate versions via workspace inheritanceVerdict: 👍 Approve-worthy. Metadata/config-only change, wired correctly, and this revision closes every actionable item from the earlier review rounds. Two small non-blocking notes below.
What's correct
Prior review concerns — all resolved ✅
Observations (non-blocking)1. 2. Dry-run artifact still open. The prior reviews asked for a fresh 3. Security / performanceNo concerns — metadata-only. No code paths, runtime deps, or feature flags changed. Codecov's file-count delta is unrelated noise. Clean iteration. The version-skew risk is now managed by tooling ( |
github-actions
Bot
removed
the
safe-to-test
|
Can you explain:
My understanding is that Simon added a workflow that will automate this for us |
cargo-release and publish-crates.yml are complementary as cargo-release handles the version bump and publish-crates.yml handles crates.io publishing in topological order, skipping already-published versions. The workflow automates what happens after the version bump, but doesn't help with the bump itself. Without cargo-release, you'd still need to manually edit 4 version entries in the root Cargo.toml, which is the version-skew risk this PR eliminates. |
3 participants
Description
Why is this change being made?
Cargo.tomlfiles, increasing the risk of operator mistakes during a release (forgetting a file or version mismatch).Cargo.toml, there are still 4 places to update in that file — someone could bumpworkspace.package.versionbut forget theworkspace.dependenciesentries.What is changing?
[workspace.package]withversionto the rootCargo.toml[workspace.dependencies]for internal crate cross-referencesaws_workload_credentials_provider,aws_workload_credentials_provider_common,aws_secretsmanager_provider, andaws_certificatemanager_providerto useversion.workspace = trueworkspace = truereferencesaws_secretsmanager_cachingretains its own independent versioncargo-releaseconfiguration so that version bumping is a single atomic command:cargo release patch --execute # or minor/majorworkspace.package.versionAND allworkspace.dependenciesentries, commits, tags, and pushes — eliminating the version-skew risk entirely.publish-crates.ymlworkflow (cargo-release haspublish = false).Related Links
Testing
How was this tested?
cargo release patch --workspace --exclude integration-tests --exclude aws_secretsmanager_cachingdry-run output:When testing locally, provide testing artifact(s):
Cargo.tomlforaws_workload_credentials_provider_commonconfirms workspace inheritance is resolved to the correct version with no path deps in the published artifactcargo-releasedry-run shows atomic bump of all 4 version sitesReviewee Checklist
Update the checklist after submitting the PR
If not, why: Local rustc (1.93) is older than required (1.94.1) for AWS SDK deps — CI will validate
If not, why: No code logic changed; only Cargo.toml metadata and config
If not, why: Requires newer rustc; CI will validate
If not, why: No user-facing behavior change
Reviewer Checklist
All reviewers please ensure the following are true before reviewing:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.