Repository navigation
Fix publishing verification when a local manifest shadows latest - #415
Merged
Merged
Conversation
Signed-off-by: Ryan Cook <rcook@redhat.com>
Reviewer's guide (collapsed on small PRs)Reviewer's GuideFix publishing verification by capturing the final platform-index digest and inspecting the immutable repository@digest reference, preventing stale local destination manifests from masking the uploaded index. The workflows now validate source artifact architectures without assembling redundant local indexes, and integration coverage exercises the shadowing scenario with documentation updated accordingly. Sequence diagram for immutable platform-index publishing verificationsequenceDiagram
participant CI as Publishing job
participant Podman as Podman
participant Registry as Container registry
CI->>Podman: podman manifest push --digestfile index.digest
Podman->>Registry: Push platform index
Registry-->>Podman: index_digest
CI->>Podman: cat index.digest
CI->>Podman: podman manifest inspect repository@index_digest
Podman->>Registry: Inspect immutable index
Registry-->>Podman: Uploaded amd64/arm64 manifests
Podman-->>CI: Verify exact child digests and platforms
Flow diagram for local destination-tag shadowing regressionflowchart LR
A[Local destination tag contains only arm64] --> B[Publish amd64 and arm64 platform index]
B --> C[Capture pushed index digest]
C --> D[Inspect repository@index_digest]
D --> E[Verify amd64 and arm64 child digests]
E --> F[Independent clients pull both platforms]
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 11 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="scripts/publish-platform-index.sh" line_range="66" />
<code_context>
fi
-podman manifest inspect "${options[@]}" "$destination" > "$scratch/index.json"
+index_digest=$(cat "$scratch/index.digest")
+[[ "$index_digest" =~ ^sha256:[0-9a-f]{64}$ ]] || { echo 'Invalid pushed index digest' >&2; exit 1; }
+# A local list named DESTINATION shadows the remote tag in manifest inspect.
+# Inspect the immutable index returned by this push, not a possibly stale local tag.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Add tests for invalid and missing digest-file output.
This validation should be covered with cases where the digest file is empty, malformed, uses an unsupported algorithm, or is absent after a nominally successful push, ensuring the script fails safely before inspecting the index.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Include comments that suggest adding tests for edge cases or invalid inputs to improve code robustness.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=99de523b-cf4c-499c-848e-2830fb0292eb&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 2
<location path="scripts/publish-platform-index.sh" line_range="65" />
<code_context>
exit 1
fi
-podman manifest inspect "${options[@]}" "$destination" > "$scratch/index.json"
+index_digest=$(cat "$scratch/index.digest")
+[[ "$index_digest" =~ ^sha256:[0-9a-f]{64}$ ]] || { echo 'Invalid pushed index digest' >&2; exit 1; }
+# A local list named DESTINATION shadows the remote tag in manifest inspect.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Avoid spawning `cat` when reading the digest file.
Bash can read the file directly, simplifying this assignment and avoiding an unnecessary subprocess:
```bash
index_digest=$(<"$scratch/index.digest")
```
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Verify that the suggestion simplifies the code logic or enhances performance, and include a code snippet demonstrating the proposed change.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=00e6741a-525d-4304-9e65-8fbc8e563bae&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 3
<location path="docs/documentation.rst" line_range="114" />
<code_context>
+
+The publisher stages amd64 and arm64 child manifests in the destination
+repository and publishes one index referencing their immutable digests.
+Verification reads the index using the digest returned by the push. A local
+manifest with the same name as a public ``latest`` tag can shadow registry
+inspection in Podman; it must not be used as evidence of published content.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Add a concrete command example showing digest-based inspection and its benefit.
This section explains the rationale but does not show how to apply it. For example, include a short equivalent such as `podman manifest push --digestfile index.digest ...` followed by `podman manifest inspect "$repository@$(cat index.digest)"`. Explicitly noting that this verifies the exact remotely pushed index, rather than a shadowed local tag, would make the guidance actionable and easier to validate independently.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Provide clear examples or code snippets to illustrate the suggested changes, and explain their benefits.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=5c09625f-903f-40dc-92f9-737652f634c6&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 4
<location path="docs/documentation.rst" line_range="117" />
<code_context>
+Verification reads the index using the digest returned by the push. A local
+manifest with the same name as a public ``latest`` tag can shadow registry
+inspection in Podman; it must not be used as evidence of published content.
+The local-registry regression includes this collision and checks that clients
+can pull both architectures from the shared tag.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Clarify which regression test demonstrates the local manifest/tag collision.
“The local-registry regression” is ambiguous to readers because no test or path is identified. Please name or link the regression test—for example, `tests/integration/publishing/test-platform-publish.sh`—so readers can find the collision scenario and its multi-architecture pull checks.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Verify that the comment addresses a potential source of confusion or ambiguity in the code or documentation.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=15391db5-3e4d-44d0-a8f4-14b1499ae578&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 5
<location path="docs/documentation.rst" line_range="117" />
<code_context>
+Verification reads the index using the digest returned by the push. A local
+manifest with the same name as a public ``latest`` tag can shadow registry
+inspection in Podman; it must not be used as evidence of published content.
+The local-registry regression includes this collision and checks that clients
+can pull both architectures from the shared tag.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Clarify what the local-registry regression tests and how the tag collision is reproduced.
“Local-registry regression” is vague to readers who do not know the test suite. Consider: “The local-registry regression reproduces this collision by creating a local manifest named `latest`, then verifies that clients can still pull both amd64 and arm64 images from the registry’s shared `latest` tag.”
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Verify that the suggestion enhances clarity and conciseness of the text, and includes a specific example or rephrasing.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=4ea295bd-cb2d-4d56-9aa4-6a934ebb653d&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 6
<location path="scripts/publish-platform-index.sh" line_range="61" />
<code_context>
# the index with --all=false; Podman 5 defaults --all to true. Keep both pinned
# child digests unchanged. Child publication and index publication are separate.
-if ! podman manifest push --all=false "${options[@]}" --format=docker "$manifest" "docker://$destination"; then
+if ! podman manifest push --all=false "${options[@]}" --format=docker --digestfile "$scratch/index.digest" "$manifest" "docker://$destination"; then
echo "Failed to publish platform index: $destination" >&2
exit 1
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Split the long publish command so digest-file handling is easier to understand and maintain.
This command now combines the publish operation, format selection, and digest-file output in one line, which increases cognitive load and makes future option changes error-prone. Consider extracting the destination and push arguments, for example: `push_args=(--all=false "${options[@]}" --format=docker --digestfile "$scratch/index.digest" "$manifest" "docker://$destination")`; then call `podman manifest push "${push_args[@]}"`. Keeping the argument construction separate makes the publish behavior easier to review without changing its semantics.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Ensure the comment addresses a specific issue or improvement in the code, such as complexity or maintainability, and provides a clear, actionable refactoring suggestion with an example and benefits.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=8482c777-a573-464d-9e75-bd548ca9b5f7&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 7
<location path=".github/workflows/docker-image.yml" line_range="1732" />
<code_context>
- - name: Assemble and verify the tested platform images
+ - name: Verify the tested platform architectures
run: |
test "$(podman image inspect --format '{{.Architecture}}' quay.io/fetchit/fetchit-amd:latest)" = amd64
test "$(podman image inspect --format '{{.Architecture}}' quay.io/fetchit/fetchit-arm:latest)" = arm64
- podman manifest add quay.io/fetchit/fetchit:latest quay.io/fetchit/fetchit-arm:latest
</code_context>
<issue_to_address>
**issue (review_instructions):** The replacement verification checks architecture but no longer verifies the child image operating system.
An amd64 or arm64 image with a non-Linux OS could pass these checks and be published into a Linux platform index. Verify both OS and architecture before publishing, for example:
```sh
test "$(podman image inspect --format '{{.Os}}/{{.Architecture}}' quay.io/fetchit/fetchit-amd:latest)" = linux/amd64
test "$(podman image inspect --format '{{.Os}}/{{.Architecture}}' quay.io/fetchit/fetchit-arm:latest)" = linux/arm64
```
This preserves the platform validation that the removed manifest inspection previously provided and prevents publishing an invalid index.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Ensure the comment identifies a potential bug risk or issue in the code and suggests a specific, actionable improvement with an explanation and code snippet.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=68cd1c97-e0eb-41f3-92a8-e4ec616def7f&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 8
<location path="scripts/publish-platform-index.sh" line_range="65" />
<code_context>
exit 1
fi
-podman manifest inspect "${options[@]}" "$destination" > "$scratch/index.json"
+index_digest=$(cat "$scratch/index.digest")
+[[ "$index_digest" =~ ^sha256:[0-9a-f]{64}$ ]] || { echo 'Invalid pushed index digest' >&2; exit 1; }
+# A local list named DESTINATION shadows the remote tag in manifest inspect.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Avoid spawning `cat` when reading the digest file.
Bash can read the file directly with `index_digest=$(<"$scratch/index.digest")`, which is simpler and avoids an unnecessary subprocess while preserving the same value.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Suggest improvements that simplify the code or make it more efficient.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=7bbb5bc0-90a9-4be4-932e-7a13f3b564df&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 9
<location path="scripts/publish-platform-index.sh" line_range="61" />
<code_context>
# the index with --all=false; Podman 5 defaults --all to true. Keep both pinned
# child digests unchanged. Child publication and index publication are separate.
-if ! podman manifest push --all=false "${options[@]}" --format=docker "$manifest" "docker://$destination"; then
+if ! podman manifest push --all=false "${options[@]}" --format=docker --digestfile "$scratch/index.digest" "$manifest" "docker://$destination"; then
echo "Failed to publish platform index: $destination" >&2
exit 1
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Extract the long publish-and-error-handling command into a helper to reduce shell complexity.
This line combines multiple concerns—publishing with several options, capturing the digest, and handling failure—making the workflow harder to scan and maintain. Consider extracting a helper such as `publish_index() { local digest_file=$1; local destination=$2; if ! podman manifest push --all=false "${options[@]}" --format=docker --digestfile "$digest_file" "$manifest" "docker://$destination"; then echo "Failed to publish platform index: $destination" >&2; return 1; fi; }`, then call it as `publish_index "$scratch/index.digest" "$destination" || exit 1`. This keeps the main flow focused on obtaining and validating the digest before inspection.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Include comments that identify complex code structures and suggest specific refactoring techniques to improve readability and maintainability; provide a clear explanation and detailed implementation example.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=24bf43e5-b41d-4c4e-85fa-b9d25a3f455f&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 10
<location path="docs/documentation.rst" line_range="117" />
<code_context>
+Verification reads the index using the digest returned by the push. A local
+manifest with the same name as a public ``latest`` tag can shadow registry
+inspection in Podman; it must not be used as evidence of published content.
+The local-registry regression includes this collision and checks that clients
+can pull both architectures from the shared tag.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Name the regression test that covers the local manifest collision.
“The local-registry regression” is not specific enough for readers to locate or verify. Please reference the concrete test path or test name, such as `tests/integration/publishing/test-platform-publish.sh`, and identify the shared-tag pull assertion it performs.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Verify that the comment includes specific details about the code, such as function names or error codes.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=d7bc6d53-67c8-4960-bb9f-cbab4e9b5e30&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 11
<location path=".github/workflows/docker-image.yml" line_range="1730" />
<code_context>
password: ${{ secrets.REGISTRY_PASSWORD }}
- - name: Assemble and verify the tested platform images
+ - name: Verify the tested platform architectures
run: |
test "$(podman image inspect --format '{{.Architecture}}' quay.io/fetchit/fetchit-amd:latest)" = amd64
</code_context>
<issue_to_address>
**issue (review_instructions):** The workflow no longer verifies that the child images are Linux images.
The checks below validate only `amd64` and `arm64`; a non-Linux image with either architecture would pass and could be published into the platform index. Restore an explicit OS check (for example, inspect `{{.Os}}` and require `linux`) or ensure the publishing script performs this validation before publishing.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Include comments that identify potential bugs or risks in the code, such as typos in critical values like IP addresses.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=59964304-38e3-4f4b-abff-d04f315cf630&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>Signed-off-by: Ryan Cook <rcook@redhat.com>
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 main publishing jobs successfully upload their platform indexes, then fail digest verification because
podman manifest inspect DESTINATION:latestreads the stale local index assembled earlier in the workflow. That index contains local-image digests rather than the pushed child digests.Capture the final push's index digest and inspect the immutable
repository@digestreference. Preserve exact child digest and platform checks. Remove redundant local destination-index assembly from the FetchIt, systemd, and Ansible publishing jobs, retaining architecture assertions for the actual artifacts on PRs and main.Extend the native local-registry regression with a conflicting local destination tag containing only arm64. Publishing must still verify the uploaded amd64/arm64 index, and independent client pulls must succeed for both platforms. Document immutable verification in the Read the Docs contributor guide.
Validation: shell syntax, workflow actionlint, and strict Sphinx checks pass locally. The native registry regression passed with the conflicting local destination index, and the Sphinx job passed. Independent Skopeo registry reads confirmed that the currently published FetchIt and systemd indexes contain both platforms with exactly the child digests reported by the successful pushes in the failing jobs. Production publication using the updated script runs after merge.
Reported jobs:
Review follow-up: validate
linux/amd64andlinux/arm64before any image push, read digest files directly in Bash, split index-push arguments for readability, and reject missing/empty digest output explicitly. Add fault tests for missing, empty, malformed, unsupported-algorithm digests and non-Linux images, with a successful-publish control. Name the regression and show digest-based commands in Read the Docs. All Sourcery threads are addressed and resolved.Updated publishing checks passed, including fault injection and native registry tests. Updated Sphinx checks passed. GitHub requires one approving review before this PR can merge and trigger the corrected production publisher.