Skip to content

add crates release - #250

Merged
simonmarty merged 10 commits into
aws:mainfrom
simonmarty:add-crates-release
Jul 23, 2026
Merged

add crates release#250
simonmarty merged 10 commits into
aws:mainfrom
simonmarty:add-crates-release

Conversation

@simonmarty

Copy link
Copy Markdown
Contributor
  • Add crates.io release workflow
  • Make idempotent

Description

Why is this change being made?

  1. Add a workflow to release crates in order.

What is changing?

  1. Created a new workflow to run integ tests, then release the crates. This uses trusted publishing.

Related Links

  • Issue #, if available:

Testing

How was this tested?

When testing locally, provide testing artifact(s):


Reviewee Checklist

Update the checklist after submitting the PR

  • I have reviewed, tested and understand all changes
    If not, why:
  • I have filled out the Description and Testing sections above
    If not, why:
  • Build and Unit tests are passing
    If not, why:
  • Unit test coverage check is passing
    If not, why:
  • Integration tests pass locally
    If not, why:
  • I have updated integration tests (if needed)
    If not, why:
  • I have ensured no sensitive information is leaking (i.e., no logging of sensitive fields, or otherwise)
    If not, why:
  • I have added explanatory comments for complex logic, new classes/methods and new tests
    If not, why:
  • I have updated README/documentation (if needed)
    If not, why:
  • I have clearly called out breaking changes (if any)
    If not, why:

Reviewer Checklist

All reviewers please ensure the following are true before reviewing:

  • Reviewee checklist has been accurately filled out
  • Code changes align with stated purpose in description
  • Test coverage adequately validates the changes

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@simonmarty
simonmarty requested a review from a team as a code owner July 17, 2026 16:46
@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.35%. Comparing base (f76ef6f) to head (442f97f).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #250   +/-   ##
=======================================
  Coverage   86.35%   86.35%           
=======================================
  Files          34       34           
  Lines        9422     9422           
  Branches     9422     9422           
=======================================
  Hits         8136     8136           
  Misses       1153     1153           
  Partials      133      133           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@secrets-manager-ai-code-review

secrets-manager-ai-code-review Bot commented Jul 17, 2026

Copy link
Copy Markdown

Claude finished @simonmarty's task in 1m 57s —— View job


Code Review: add crates release

  • Read the new workflow file
  • Verify crate publish ordering against Cargo.toml dependencies
  • Check the embedded bash script for correctness
  • Review security/permissions posture

🔴 Blocking: the script is truncated — the for loop is never closed

.github/workflows/publish-crates.yml ends at line 71 with fi. The for crate in ...; do opened on line 59 has no matching done, and the ::group:: opened on line 62 has no ::endgroup::. The file's last bytes are literally f i \n.

          for crate in "${crates[@]}"; do
            ...
            if [[ "$http_code" == "200" ]]; then
              echo "$crate@$version already published — skipping"
            else
              cargo publish -p "$crate"
            fi
            # ⬅️ missing: done   (and an "::endgroup::" before it)

Because set -euo pipefail runs under bash, this will fail at parse time with syntax error: unexpected end of file, so the publish step never runs. This needs:

              cargo publish -p "$crate"
            fi
            echo "::endgroup::"
          done

Fix this →

(Note: I can't push this fix myself — GitHub App permissions don't allow modifying files under .github/workflows/.)


✅ What looks good

  • Publish order is correct. Verified against the path/version deps in each Cargo.toml:
    • common and aws_secretsmanager_caching have no internal deps (publish first ✓)
    • aws_certificatemanager_providercommon
    • aws_secretsmanager_providercaching + common
    • aws_workload_credentials_providercommon + certificatemanager + secretsmanager
  • integration-tests is correctly excluded (it has publish = false and isn't in the list).
  • Trusted publishing via rust-lang/crates-io-auth-action + id-token: write + environment: publish-crates gating is the right pattern.
  • All third-party actions are pinned to commit SHAs. 👍

🟡 Suggestions (non-blocking)

  1. Idempotency check treats every non-200 as "not published." A transient 429/5xx from the crates.io API would fall through to cargo publish. Consider gating specifically on 404 for the skip decision, e.g. if [[ "$http_code" == "200" ]] ... elif [[ "$http_code" != "404" ]]; then echo "unexpected HTTP $http_code" >&2; exit 1; fi, so a flaky API response fails loudly instead of attempting a publish.

  2. Index-propagation between dependent crates. After publishing a dependency, the next crate depends on it being in the registry index. Modern cargo publish waits for the crate to appear before returning, so this is usually fine — but if you ever see intermittent "no matching package" failures, that's the cause. Worth a comment noting the reliance on cargo's wait behavior.

  3. No concurrency group. Two overlapping runs (e.g. a manual dispatch during a release) could race on the same version. Low risk given this is workflow_dispatch/workflow_call, but a concurrency: { group: publish-crates, cancel-in-progress: false } would be cheap insurance.

  4. egress-policy: audit (line 33) only logs egress. Once you've observed the required endpoints (crates.io, the token exchange), consider tightening to block with an allowlist for defense-in-depth.


Overall the approach is solid — the only thing standing between this and a working release is the truncated script in the publish step.

@secrets-manager-ai-code-review

secrets-manager-ai-code-review Bot commented Jul 17, 2026

Copy link
Copy Markdown

Claude finished @simonmarty's task in 58s —— View job


Code Review: add crates release

  • Read the updated workflow file
  • Verify the previously-reported truncated-loop issue is resolved
  • Verify crate publish ordering against Cargo.toml deps
  • Review security / correctness / idempotency

✅ Blocking issue from the prior review is resolved

The truncated script has been fixed. The for loop now closes correctly at line 73 with done, and the ::group:: opened on line 62 is matched by echo "::endgroup::" on line 72. The bash -euo pipefail script now parses and runs.

✅ What looks good

  • Publish order is correct. Re-verified against the path deps in each Cargo.toml:
    • aws_workload_credentials_provider_common (3.1.0) — no internal deps → published first ✓
    • aws_secretsmanager_caching (2.1.0) — no internal deps ✓
    • aws_certificatemanager_provider (3.1.0) → common
    • aws_secretsmanager_provider (3.1.0) → caching + common
    • aws_workload_credentials_provider (3.1.0) → common + certificatemanager + secretsmanager
  • integration-tests is correctly excluded (it isn't in the list).
  • Trusted publishing via rust-lang/crates-io-auth-action + id-token: write + environment: publish-crates gating is the right pattern.
  • All third-party actions are pinned to commit SHAs. 👍
  • Idempotency via the curl HTTP-code pre-check reads cleanly and lets the workflow be safely re-run.

🟡 Suggestions (non-blocking)

  1. Idempotency check treats every non-200 as "not published." A transient 429/5xx from the crates.io API falls through to cargo publish. Consider gating the skip decision specifically on 404, so a flaky API response fails loudly instead of attempting a publish:

    if [[ "$http_code" == "200" ]]; then
      echo "$crate@$version already published — skipping"
    elif [[ "$http_code" == "404" ]]; then
      cargo publish -p "$crate"
    else
      echo "unexpected HTTP $http_code from crates.io for $crate@$version" >&2
      exit 1
    fi

    Fix this →

  2. No concurrency group. Two overlapping runs (e.g. a manual workflow_dispatch during a release) could race on the same version. Adding concurrency: { group: publish-crates, cancel-in-progress: false } is cheap insurance.

  3. egress-policy: audit (line 33) only logs egress. Once the required endpoints (crates.io + the token-exchange host) are observed in the audit logs, consider tightening to block with an allowlist for defense-in-depth.

  4. Index-propagation between dependent crates. After publishing a dependency, the next crate needs it in the registry index. Modern cargo publish waits for the crate to appear before returning, so this is generally fine — but a brief comment noting the reliance on cargo's wait behavior would help future readers if intermittent "no matching package" errors ever appear.


Overall this looks good to go — the prior blocking issue is fixed, ordering is correct, and the security posture (SHA-pinned actions, trusted publishing, environment gating) is solid. The remaining items are all optional hardening.

@i-am-SR i-am-SR left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following seem worth addressing and easy fixes:

1. **Idempotency check treats every non-200 as "not published."** A transient `429`/`5xx` from the crates.io API falls through to `cargo publish`. Consider gating the skip decision specifically on `404`, so a flaky API response fails loudly instead of attempting a publish:
   ```shell
   if [[ "$http_code" == "200" ]]; then
     echo "$crate@$version already published — skipping"
   elif [[ "$http_code" == "404" ]]; then
     cargo publish -p "$crate"
   else
     echo "unexpected HTTP $http_code from crates.io for $crate@$version" >&2
     exit 1
   fi
   ```
   [Fix this →](https://claude.ai/code?q=In%20.github%2Fworkflows%2Fpublish-crates.yml%20on%20branch%20add-crates-release%2C%20the%20idempotency%20check%20in%20the%20%22Publish%20crates%22%20step%20treats%20any%20non-200%20HTTP%20code%20as%20%22not%20published%22.%20Change%20it%20so%20only%20404%20triggers%20cargo%20publish%2C%20200%20skips%2C%20and%20any%20other%20code%20fails%20the%20step%20with%20an%20error%20message.&repo=aws/aws-workload-credentials-provider)

2. **No `concurrency` group.** Two overlapping runs (e.g. a manual `workflow_dispatch` during a release) could race on the same version. Adding `concurrency: { group: publish-crates, cancel-in-progress: false }` is cheap insurance.

Comment thread .github/workflows/publish-crates.yml
Comment thread .github/workflows/publish-crates.yml
derik01
derik01 previously approved these changes Jul 20, 2026
Comment thread .github/workflows/publish-crates.yml Outdated
@derik01 derik01 added the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label Jul 20, 2026
@github-actions github-actions Bot removed the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label Jul 20, 2026
@secrets-manager-ai-code-review

secrets-manager-ai-code-review Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @simonmarty's task in 35s —— View job


Code Review: add crates release

  • Read the updated workflow file
  • Verify prior blocking/idempotency issues are resolved
  • Verify crate publish ordering against Cargo.toml deps
  • Review security / correctness / open review threads

✅ Resolved since earlier reviews

  • Truncated for loop — fixed; loop closes at line 76 with done, ::group:: matched by ::endgroup:: on line 75.
  • Idempotency now gates on 404 (lines 67–74): 200 skips, 404 publishes, anything else fails loudly with exit 1. This addresses the transient-429/5xx concern from the prior reviews. 👍
  • Publish order is correct (topological): commoncachingcertificatemanager_providersecretsmanager_providerworkload_credentials_provider. integration-tests correctly excluded.
  • Trusted publishing (crates-io-auth-action + id-token: write + environment: publish-crates) and SHA-pinned actions — solid posture.

🟡 Still open (non-blocking)

  1. No concurrency group. Two overlapping runs could race on the same version. Cheap insurance:

    concurrency:
      group: publish-crates
      cancel-in-progress: false

    Fix this →

  2. fetch-depth: 1 publishes the branch HEAD (lines 35–38). This ties into @i-am-SR's thread: as a standalone workflow_dispatch, HEAD may have drifted past the released commit. You mentioned restricting to a git tag via environment settings — worth confirming the publish-crates environment enforces a tag/deployment-branch policy, otherwise a dispatch from an arbitrary ref will publish whatever is checked out. A ref: input to actions/checkout (e.g. the release tag) would make this explicit and robust.

  3. Per-job permissions (per @derik01's nit). pull-requests: write (line 18) is only needed by the reusable integration-tests workflow, but it's currently granted at the top level, so the publish job inherits it too. Moving id-token: write/contents: read to the publish job and dropping pull-requests: write from the publish job scope narrows the token. Note: workflow_call'd workflows manage their own permissions, so scoping here is low-cost.

  4. egress-policy: audit (line 33) only logs egress. Once the required endpoints (crates.io + token-exchange host) are observed, tightening to block with an allowlist adds defense-in-depth.

  5. Index-propagation comment. Dependent crates rely on cargo publish waiting for the dependency to appear in the index before returning (which modern cargo does). A one-line comment noting this reliance would help future readers if intermittent "no matching package" errors ever surface.


Overall this is in good shape — the earlier blocking issue and the idempotency concern are both resolved, ordering and the trusted-publishing setup are correct. The remaining items are optional hardening plus the tag/ref checkout question worth settling before this becomes part of the release flow.

i-am-SR
i-am-SR previously approved these changes Jul 22, 2026

@i-am-SR i-am-SR left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's keep track of open non-blocking issues (from Claude's review) #1, #2, #3 and resolve them in a follow up.

@simonmarty

Copy link
Copy Markdown
Contributor Author
  1. And 3. are done. I don't want to do address 2. because ideally I want a GH actions run to be tied to a commit and rely on the environment settings to prevent a run from happening on other branches. This helps with auditing and ensuring that retries of a workflow always fetch the same commit even if the main branch has moved forward.

@simonmarty simonmarty added the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label Jul 23, 2026
@secrets-manager-ai-code-review

secrets-manager-ai-code-review Bot commented Jul 23, 2026

Copy link
Copy Markdown

Claude encountered an error after 3m 26s —— View job


I'll analyze this and get back to you.

@github-actions github-actions Bot removed the safe-to-test Maintainer approval to run integration tests for external contributor PRs. label Jul 23, 2026
@simonmarty
simonmarty merged commit 4887d82 into aws:main Jul 23, 2026
9 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants