Uh oh!
There was an error while loading. Please reload this page.
Make --rust-image-digest optional for build_image.py - #29
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds automatic resolution of the Rust base image digest (from builds.json) when the selected Rust label maps to a single declared digest, making --rust-image-digest optional and only required for ambiguous labels.
Changes:
- Introduce
builds.resolve_rust_digest()to validate/resolve digests for(stellar-cli version, rust label)selections. - Update
build_image.pyCLI to make--rust-image-digestoptional and use the new resolver. - Add unit tests and update docs to reflect the new default behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_builds.py | Adds unit tests for digest resolution behavior (implicit, explicit, ambiguous, undeclared). |
| tests/unit/test_build_image.py | Adds CLI-level tests for inferred digest and failure on ambiguity without digest. |
| scripts/lib/builds.py | Implements digest resolution logic for rust base pins. |
| scripts/build_image.py | Makes --rust-image-digest optional and resolves digest from builds data. |
| RELEASE.md | Updates release/build instructions to reflect optional digest selection. |
| README.md | Updates local build documentation to reflect automatic digest resolution. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| entry = find_cli(data, cli) | ||
| pins = [pin for pin in (entry or {}).get("rust_versions", []) if label_of(pin) == label] | ||
| if not pins: | ||
| raise ValueError( | ||
| f"stellar-cli {cli} is not declared with rust base {label} in builds.json" | ||
| ) |
| if digest: | ||
| assert_pair_declared(data, cli, f"{label}@{digest}") | ||
| return digest |
| assert rc == 0 | ||
| args = captured.call_args[0][0] | ||
| assert f"RUST_IMAGE_DIGEST={DIGEST}" in args |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:b8f6ee6a97
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ./scripts/smoke_test_image.py \ | ||
| --image "${{ steps.pair.outputs.image }}" \ | ||
| --stellar-cli-version "${{ steps.pair.outputs.cli }}" \ | ||
| --rust-version "${{ steps.pair.outputs.rust }}" \ |
There was a problem hiding this comment.
In the build workflow that is invoked by .github/workflows/ci.yml, this smoke-test command now omits --rust-image-digest, but scripts/smoke_test_image.py still declares that option as required in build_parser(). As soon as CI reaches this step, argparse exits with a missing-argument error before any smoke checks run, so the build job fails even for valid images; either keep passing ${{ steps.pair.outputs.digest }} here or make the smoke script resolve it too.
Useful? React with 👍 / 👎.
Pasting in a 64-char sha256 every single time you run the build command is annoying — and honestly kind of pointless, since most of the time the digest is already implied by the other args you're passing.
builds.jsonis already the source of truth of the (stellar-cli, rust base) pairs we publish, and every rust base is pinned as a full<label>@<digest>. In the common case, a given cli version only declares a rust version once, so--stellar-cli-version+--rust-versionalready pin down the digest completely. Making you copy-paste it on top of that doesn't buy any safety, it just gives you one more chance to fat-finger a hash.The digest only actually tells us something when the same rust version shows up with more than one digest (say, a relabelled or refreshed base). So that's the only case where the flag is still required now. The updated behavior looks like:
builds.jsonfor you.So nothing's been loosened — we've just dropped a required argument in the one case where the data already knew the answer.