Fix digest show by bootc status and the one reported on the disk image#58
Merged
Conversation
The node image needs to be pulled remotely and we should avoid all possible local reference to the image, in order to embed in the disk image the correct digest. This digest will be, then reported by bootc status, and we want to match it with the digest of the remote image. Signed-off-by: Alice Frosi <afrosi@redhat.com>
Assisted-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Alice Frosi <afrosi@redhat.com>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideEnsure the disk image is built from the exact bootc image and digest that were pushed, by exporting the push destination from the push step, cleaning up local images, and adjusting the Makefile dependency so the disk image build can use an explicitly provided image instead of always rebuilding it. Flow diagram for updated bootc and disk image build pipelineflowchart TD
A[push-bootc step
podman push BOOTC_SRC to PUSH_DEST:TAG] --> B[Read /tmp/bootc-digest
set output digest]
B --> C[podman rmi BOOTC_SRC
podman rmi PUSH_DEST:TAG
podman rmi PUSH_DEST:latest]
C --> D[Set output push_dest=PUSH_DEST:TAG]
D --> E[Build disk image step]
B --> E
E --> F{BOOTC_DIGEST and PUSH_DEST set?}
F -- yes --> G[podman pull PUSH_DEST]
G --> H[make build-disk-image
BOOTC_IMAGE=PUSH_DEST
BOOTC_DIGEST=BOOTC_DIGEST]
F -- no --> I[make build-disk-image
without BOOTC_IMAGE and BOOTC_DIGEST]
H --> J[Disk image built from pushed bootc image]
I --> J
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In the cleanup step, consider quoting the image variables in
podman rmi -f ${BOOTC_SRC} ${PUSH_DEST}:${TAG} ${PUSH_DEST}:latestor guarding against empty values so that missing/empty vars don’t cause unexpected word splitting or a hard failure of the step. - The
push_destoutput now includes the tag (i.e.,PUSH_DEST:${TAG}) while the variable name still suggests a bare repository; consider renaming this output (e.g.,push_ref) or adding a separate output for the untagged destination to avoid confusion for future consumers. - By removing the
build-bootc-imagedependency from thebuild-disk-imageMake target,make build-disk-imagenow assumes the bootc image already exists; ensure this is intentional for local usage or consider adding a separate target to preserve the previous behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the cleanup step, consider quoting the image variables in `podman rmi -f ${BOOTC_SRC} ${PUSH_DEST}:${TAG} ${PUSH_DEST}:latest` or guarding against empty values so that missing/empty vars don’t cause unexpected word splitting or a hard failure of the step.
- The `push_dest` output now includes the tag (i.e., `PUSH_DEST:${TAG}`) while the variable name still suggests a bare repository; consider renaming this output (e.g., `push_ref`) or adding a separate output for the untagged destination to avoid confusion for future consumers.
- By removing the `build-bootc-image` dependency from the `build-disk-image` Make target, `make build-disk-image` now assumes the bootc image already exists; ensure this is intentional for local usage or consider adding a separate target to preserve the previous behavior.
## Individual Comments
### Comment 1
<location path=".github/workflows/build-node-image.yaml" line_range="73" />
<code_context>
echo "digest=${BOOTC_DIGEST}" >> "$GITHUB_OUTPUT"
echo "Bootc image pushed with digest: ${BOOTC_DIGEST}"
+ echo "Clean up local images"
+ podman rmi -f ${BOOTC_SRC} ${PUSH_DEST}:${TAG} ${PUSH_DEST}:latest
+ echo "push_dest=${PUSH_DEST}:${TAG}" >> "$GITHUB_OUTPUT"
</code_context>
<issue_to_address>
**issue (bug_risk):** Quote variables in `podman rmi` to avoid word-splitting and globbing issues.
Unquoted variables here risk word-splitting, globbing, and misbehaviour when values are empty or contain spaces/special characters. For example:
```sh
echo "Clean up local images"
podman rmi -f "${BOOTC_SRC}" "${PUSH_DEST}:${TAG}" "${PUSH_DEST}:latest"
```
If you want best-effort cleanup that doesn’t fail when an image is missing, consider `podman rmi --ignore` instead of relying on `-f` alone.
</issue_to_address>
### Comment 2
<location path="node-images/fedora/Makefile" line_range="28" />
<code_context>
@echo "✅ Bootc image built: $(BOOTC_IMAGE)"
# Build the disk image (qcow2 in a scratch container, for use as a podman image volume)
-build-disk-image: build-bootc-image
+build-disk-image:
@echo "=== Building node image with qcow2 disk ==="
STORAGE_PATH=$$(podman info --format '{{.Store.GraphRoot}}') && \
</code_context>
<issue_to_address>
**suggestion:** Dropping the `build-bootc-image` prerequisite may degrade the local developer experience.
With `build-disk-image` no longer depending on `build-bootc-image`, running `make build-disk-image` won’t rebuild the bootc image unless the user does it manually or passes `BOOTC_IMAGE`/`BOOTC_DIGEST`. If CI now orchestrates both targets, consider splitting them: keep `build-disk-image` depending on `build-bootc-image` for local use, and introduce a separate CI-oriented target (e.g., `build-disk-image-ci`) without that prerequisite.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| echo "digest=${BOOTC_DIGEST}" >> "$GITHUB_OUTPUT" | ||
| echo "Bootc image pushed with digest: ${BOOTC_DIGEST}" | ||
| echo "Clean up local images" | ||
| podman rmi -f ${BOOTC_SRC} ${PUSH_DEST}:${TAG} ${PUSH_DEST}:latest |
There was a problem hiding this comment.
issue (bug_risk): Quote variables in podman rmi to avoid word-splitting and globbing issues.
Unquoted variables here risk word-splitting, globbing, and misbehaviour when values are empty or contain spaces/special characters. For example:
echo "Clean up local images"
podman rmi -f "${BOOTC_SRC}" "${PUSH_DEST}:${TAG}" "${PUSH_DEST}:latest"If you want best-effort cleanup that doesn’t fail when an image is missing, consider podman rmi --ignore instead of relying on -f alone.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The digest reported on the disk image still doesn't match with digested reported by bootc status once boooted. This PR ensure that there are no local images left and the base image is freshly pulled from the registry