Repository navigation
Fix image publishing using pushed digests and test the registry path - #407
Merged
Merged
Conversation
Signed-off-by: Ryan Cook <rcook@redhat.com>
Reviewer's GuideThe PR replaces locally assembled, potentially stale platform indexes with a reusable publisher that records and verifies the exact digests returned by architecture pushes, updates all production image publishing jobs to use it, and adds a local-registry Actions regression test covering archive reloads, stale tags, index integrity, and multi-architecture pulls. Sequence diagram for digest-pinned platform image publishingsequenceDiagram
participant Workflow as GitHub Actions workflow
participant Publisher as publish-platform-index.sh
participant Registry as Container registry
Workflow->>Publisher: bash scripts/publish-platform-index.sh destination amd_image arm_image
Publisher->>Publisher: podman image inspect
Publisher->>Registry: podman push amd_image to repository-amd:latest
Registry-->>Publisher: amd.digest
Publisher->>Registry: podman push arm_image to repository-arm:latest
Registry-->>Publisher: arm.digest
Publisher->>Publisher: podman manifest create
Publisher->>Publisher: podman manifest add repository-amd@amd_digest
Publisher->>Publisher: podman manifest add repository-arm@arm_digest
Publisher->>Registry: podman manifest push destination:latest
Publisher->>Registry: podman manifest inspect destination:latest
Registry-->>Publisher: platform entries and digests
Publisher->>Publisher: verify amd64 and arm64 entries match pushed digests
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Signed-off-by: Ryan Cook <rcook@redhat.com>
![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 24 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="29" />
<code_context>
+podman manifest create "$manifest"
+podman manifest add "${options[@]}" "$manifest" "docker://$repository-amd@$amd_digest"
+podman manifest add "${options[@]}" "$manifest" "docker://$repository-arm@$arm_digest"
+podman manifest push --all "${options[@]}" --format=docker "$manifest" "docker://$destination"
+podman manifest inspect "${options[@]}" "docker://$destination" > "$scratch/index.json"
+python3 - "$scratch/index.json" "$amd_digest" "$arm_digest" <<'PYTHON'
</code_context>
<issue_to_address>
**Concurrent publishes leave stale indexes**
When two publishing runs for the same destination overlap, the older run can push its index after the newer run and replace `:latest` with stale digests. Also, `manifest inspect` can read the other run’s index and fail the digest assertion, so users get stale content or a successful publish is reported as failed.
Serialize publishing runs that target the same destination.
Also at `scripts/publish-platform-index.sh:30-38`.
</issue_to_address>
### Comment 2
<location path=".github/workflows/platform-publish.yml" line_range="35-38" />
<code_context>
+ podman load -i /tmp/platforms.tar
+ # Seed an incorrect old architecture tag: the script must replace it.
+ podman push --tls-verify=false localhost/publish-source:arm64 docker://127.0.0.1:5000/fetchit-test-amd:latest
+ FETCHIT_REGISTRY_TLS_VERIFY=false bash scripts/publish-platform-index.sh \
+ 127.0.0.1:5000/fetchit-test:latest localhost/publish-source:amd64 localhost/publish-source:arm64
+ # Moving an architecture tag afterward must not change index membership.
+ podman push --tls-verify=false localhost/publish-source:arm64 docker://127.0.0.1:5000/fetchit-test-amd:latest
+ for arch in amd64 arm64; do
+ loaded=$(podman pull --quiet --tls-verify=false --policy=always --platform "linux/$arch" 127.0.0.1:5000/fetchit-test:latest)
</code_context>
<issue_to_address>
**Stale architecture tags pass the check**
When the publisher leaves the amd tag stale, the workflow overwrites the amd tag with the arm image after publishing, then checks only pulls from the multi-architecture index. The stale tag is therefore never detected, and the regression check passes.
Check the amd tag immediately after publishing, before overwriting it to test index membership.
Also at `.github/workflows/platform-publish.yml:41`.
</issue_to_address>
### Comment 3
<location path=".github/workflows/platform-publish.yml" line_range="21" />
<code_context>
+ export XDG_RUNTIME_DIR="/run/user/$(id -u)"
+ podman run -d --rm --name publish-registry --network host \
+ -e REGISTRY_HTTP_ADDR=127.0.0.1:5000 \
+ docker.io/library/registry:2@sha256:a3d8aaa63ed8681a604f1dea0aa03f100d5895b6a58ace528858a7b332415373
+ trap 'podman logs publish-registry; podman rm -f publish-registry' EXIT
+ timeout 30 bash -c 'until curl --fail --silent http://127.0.0.1:5000/v2/; do sleep 1; done'
</code_context>
<issue_to_address>
**Anonymous pulls block regression checks**
When docker Hub is unavailable or rate-limits anonymous pulls, the workflow’s registry startup and the fixture’s Alpine build pull images directly from Docker Hub without credentials or a fallback. Those pulls fail before `publish-platform-index.sh` runs, so the regression job goes red even when publishing is correct.
Use mirrored or otherwise locally available registry and fixture images instead of relying on anonymous Docker Hub pulls.
Also at `tests/integration/publishing/Containerfile:1`.
</issue_to_address>
### Comment 4
<location path="scripts/publish-platform-index.sh" line_range="8" />
<code_context>
+destination=$1 amd_image=$2 arm_image=$3
+[[ "$destination" == *:latest ]] || { echo 'Destination must include :latest' >&2; exit 1; }
+repository=${destination%:latest}
+case "${FETCHIT_REGISTRY_TLS_VERIFY:-true}" in true|false) ;; *) echo 'Invalid TLS verification setting' >&2; exit 1;; esac
+options=(--tls-verify="${FETCHIT_REGISTRY_TLS_VERIFY:-true}")
+if [[ -n ${FETCHIT_PUBLISH_AUTH_FILE:-} ]]; then options+=(--authfile="$FETCHIT_PUBLISH_AUTH_FILE"); fi
</code_context>
<issue_to_address>
**issue (review_instructions):** Allowing TLS verification to be disabled for arbitrary registries creates a man-in-the-middle risk.
The regression test needs an insecure local registry, but this option can also be used accidentally against a remote production registry. Restrict `false` to loopback destinations (for example, reject it unless the registry host is `127.0.0.1`/`localhost`), or use a locally generated CA and keep verification enabled in the test. A safer guard would be: `if [[ ${FETCHIT_REGISTRY_TLS_VERIFY:-true} == false && $destination != 127.0.0.1:* && $destination != localhost:* ]]; then echo 'TLS verification may only be disabled for local registries' >&2; exit 1; fi`
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Include comments that identify potential security risks and provide a safer alternative.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=77cf81e0-aaed-48cb-bc7b-43cb0ecdb17b&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 5
<location path="scripts/publish-platform-index.sh" line_range="4" />
<code_context>
+#!/usr/bin/env bash
+# Publish only the supplied, tested images; pin index entries to the pushed digests.
+set -euo pipefail
+[[ $# -eq 3 ]] || { echo 'Usage: publish-platform-index.sh DESTINATION:latest AMD64_IMAGE ARM64_IMAGE' >&2; exit 1; }
+destination=$1 amd_image=$2 arm_image=$3
+[[ "$destination" == *:latest ]] || { echo 'Destination must include :latest' >&2; exit 1; }
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Add regression tests for the script's invalid-input paths.
The integration workflow currently exercises only a successful publish. Please add tests covering an incorrect argument count, a destination without `:latest`, an invalid `FETCHIT_REGISTRY_TLS_VERIFY` value, and images with incorrect architectures or missing local references.
<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 6
<location path="scripts/publish-platform-index.sh" line_range="38" />
<code_context>
+ manifests = json.load(source)['manifests']
+expected = {('linux', 'amd64'): sys.argv[2], ('linux', 'arm64'): sys.argv[3]}
+actual = {(m['platform']['os'], m['platform']['architecture']): m['digest'] for m in manifests}
+assert len(manifests) == 2 and actual == expected, (actual, expected)
+PYTHON
</code_context>
<issue_to_address>
**issue (review_instructions):** Verification can be disabled when Python runs with optimizations.
`assert` statements are skipped under `python3 -O`, allowing an incorrect remote index to pass verification. Replace this with an explicit conditional that raises an exception (for example, `if len(manifests) != 2 or actual != expected: raise RuntimeError(...)`) so the check is always enforced.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Include comments that identify potential bug risks and suggest specific improvements.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=90a0c841-e7ee-4f2b-b5a4-fd6d648d4413&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 7
<location path="scripts/publish-platform-index.sh" line_range="8" />
<code_context>
+destination=$1 amd_image=$2 arm_image=$3
+[[ "$destination" == *:latest ]] || { echo 'Destination must include :latest' >&2; exit 1; }
+repository=${destination%:latest}
+case "${FETCHIT_REGISTRY_TLS_VERIFY:-true}" in true|false) ;; *) echo 'Invalid TLS verification setting' >&2; exit 1;; esac
+options=(--tls-verify="${FETCHIT_REGISTRY_TLS_VERIFY:-true}")
+if [[ -n ${FETCHIT_PUBLISH_AUTH_FILE:-} ]]; then options+=(--authfile="$FETCHIT_PUBLISH_AUTH_FILE"); fi
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Split the TLS setting validation into readable statements
This compressed `case` expression is difficult to scan and extend. Consider making the default explicit and formatting the validation separately, for example:
```bash
registry_tls_verify=${FETCHIT_REGISTRY_TLS_VERIFY:-true}
if [[ "$registry_tls_verify" != true && "$registry_tls_verify" != false ]]; then
echo 'Invalid TLS verification setting' >&2
exit 1
fi
options=("--tls-verify=$registry_tls_verify")
```
This makes the accepted values and the value passed to Podman independently visible, and makes the logic easier to test or modify later.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Suggest actionable refactoring or code improvements that enhance readability, maintainability, or testability, with clear examples and 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 8
<location path=".github/workflows/platform-publish.yml" line_range="24" />
<code_context>
+ docker.io/library/registry:2@sha256:a3d8aaa63ed8681a604f1dea0aa03f100d5895b6a58ace528858a7b332415373
+ trap 'podman logs publish-registry; podman rm -f publish-registry' EXIT
+ timeout 30 bash -c 'until curl --fail --silent http://127.0.0.1:5000/v2/; do sleep 1; done'
+ for arch in amd64 arm64; do
+ podman build --format oci --platform "linux/$arch" -t "localhost/publish-source:$arch" \
+ -f tests/integration/publishing/Containerfile tests/integration/publishing
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Move the local-registry regression scenario into a test script
This step embeds the entire registry setup, archive round-trip, publishing, and verification scenario in one workflow block. Extracting it to a repository script (for example, `tests/integration/publishing/run-local-registry-test.sh`) and invoking it here would allow the scenario to be run locally and tested independently:
```yaml
- name: Exercise platform publishing
run: bash tests/integration/publishing/run-local-registry-test.sh
```
The script can retain the existing `trap` and assertions while the workflow only describes how the test is invoked. This improves readability and makes failures reproducible without requiring a GitHub Actions run.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Suggest actionable refactoring or code improvements that enhance readability, maintainability, or testability, with clear examples and 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 9
<location path=".github/workflows/platform-publish.yml" line_range="33" />
<code_context>
+ podman save --multi-image-archive -o /tmp/platforms.tar localhost/publish-source:amd64 localhost/publish-source:arm64
+ podman image rm -f "$amd" "$arm"
+ podman load -i /tmp/platforms.tar
+ # Seed an incorrect old architecture tag: the script must replace it.
+ podman push --tls-verify=false localhost/publish-source:arm64 docker://127.0.0.1:5000/fetchit-test-amd:latest
+ FETCHIT_REGISTRY_TLS_VERIFY=false bash scripts/publish-platform-index.sh \
</code_context>
<issue_to_address>
**issue (review_instructions):** This comment describes the test setup but does not suggest an implementation approach or alternative.
Consider explaining the expected replacement behavior or suggesting that the test assert the stale tag is overwritten, so the comment provides actionable context about the intended improvement.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Ensure the comment provides a suggestion or alternative approach to improve the code.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=8e30c23b-05d2-4801-ac7f-f4e2378f6a6c&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 10
<location path=".github/workflows/platform-publish.yml" line_range="37" />
<code_context>
+ podman push --tls-verify=false localhost/publish-source:arm64 docker://127.0.0.1:5000/fetchit-test-amd:latest
+ FETCHIT_REGISTRY_TLS_VERIFY=false bash scripts/publish-platform-index.sh \
+ 127.0.0.1:5000/fetchit-test:latest localhost/publish-source:amd64 localhost/publish-source:arm64
+ # Moving an architecture tag afterward must not change index membership.
+ podman push --tls-verify=false localhost/publish-source:arm64 docker://127.0.0.1:5000/fetchit-test-amd:latest
+ for arch in amd64 arm64; do
</code_context>
<issue_to_address>
**issue (review_instructions):** This comment states an invariant but offers no suggestion or alternative approach.
Consider wording it to explain the safer approach being tested—for example, that the index should remain pinned to the digests captured during the initial publish rather than resolving mutable tags again.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Ensure the comment provides a suggestion or alternative approach to improve the code.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=8e30c23b-05d2-4801-ac7f-f4e2378f6a6c&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 11
<location path="scripts/publish-platform-index.sh" line_range="2" />
<code_context>
+#!/usr/bin/env bash
+# Publish only the supplied, tested images; pin index entries to the pushed digests.
+set -euo pipefail
+[[ $# -eq 3 ]] || { echo 'Usage: publish-platform-index.sh DESTINATION:latest AMD64_IMAGE ARM64_IMAGE' >&2; exit 1; }
</code_context>
<issue_to_address>
**issue (review_instructions):** The comment documents behavior but does not provide a suggestion or alternative approach.
Consider stating why digest pinning is preferred over resolving mutable architecture tags, such as preventing stale or retagged images from entering the platform index.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Ensure the comment provides a suggestion or alternative approach to improve the code.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=8e30c23b-05d2-4801-ac7f-f4e2378f6a6c&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 12
<location path=".github/workflows/docker-image.yml" line_range="1806" />
<code_context>
- podman push --authfile ~/.docker/config.json quay.io/fetchit/fetchit-arm:latest
- podman push --authfile ~/.docker/config.json quay.io/fetchit/fetchit-amd:latest
- podman manifest push --all --authfile ~/.docker/config.json --format=docker quay.io/fetchit/fetchit:latest docker://quay.io/fetchit/fetchit:latest
+ FETCHIT_PUBLISH_AUTH_FILE="$HOME/.docker/config.json" bash scripts/publish-platform-index.sh \
+ quay.io/fetchit/fetchit:latest quay.io/fetchit/fetchit-amd:latest quay.io/fetchit/fetchit-arm:latest
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Centralize the repeated platform-publish step to improve workflow organization.
The three publish jobs in `.github/workflows/docker-image.yml` now repeat the same environment setup and script invocation structure. Consider introducing a reusable workflow/action or a shared composite step for this operation, parameterized by the destination and architecture image names. This would keep the jobs consistent and make future changes to publishing behavior less error-prone.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Ensure the comment provides a clear suggestion for improving the organization or structure of the code or documentation, with rationale and specific context.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=5823e292-3e9c-49a0-ad36-4eb208fb9cbd&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 13
<location path="scripts/publish-platform-index.sh" line_range="38" />
<code_context>
+ manifests = json.load(source)['manifests']
+expected = {('linux', 'amd64'): sys.argv[2], ('linux', 'arm64'): sys.argv[3]}
+actual = {(m['platform']['os'], m['platform']['architecture']): m['digest'] for m in manifests}
+assert len(manifests) == 2 and actual == expected, (actual, expected)
+PYTHON
</code_context>
<issue_to_address>
**issue (review_instructions):** Use explicit validation instead of `assert` for verifying the remote index.
Assertions can be disabled with Python's `-O` flag, allowing an invalid or incomplete index to pass silently. Replace this with an explicit check that prints the actual and expected entries to stderr and exits nonzero, for example: `if len(manifests) != 2 or actual != expected:\n raise SystemExit(f'Unexpected platform index: actual={actual!r}, expected={expected!r}')`.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
The comment should address potential issues such as error handling or code robustness.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=5704ccf1-6084-47b5-9fbd-06d8a0441c56&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 14
<location path=".github/workflows/platform-publish.yml" line_range="24" />
<code_context>
+ docker.io/library/registry:2@sha256:a3d8aaa63ed8681a604f1dea0aa03f100d5895b6a58ace528858a7b332415373
+ trap 'podman logs publish-registry; podman rm -f publish-registry' EXIT
+ timeout 30 bash -c 'until curl --fail --silent http://127.0.0.1:5000/v2/; do sleep 1; done'
+ for arch in amd64 arm64; do
+ podman build --format oci --platform "linux/$arch" -t "localhost/publish-source:$arch" \
+ -f tests/integration/publishing/Containerfile tests/integration/publishing
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The regression test does not verify that the seeded architecture tag was actually replaced.
The test seeds `fetchit-test-amd:latest` with the arm64 image, but only validates the resulting index's platform pulls. Add an explicit digest assertion for the architecture tag after publishing, for example: `amd_tag=$(podman pull --quiet --tls-verify=false --policy=always 127.0.0.1:5000/fetchit-test-amd:latest); test "$(podman image inspect --format '{{.Digest}}' "$amd_tag")" = "$amd_digest"` (or compare the remote manifest digest directly). This ensures the replacement behavior being described is covered independently of index membership.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Verify that the suggestion addresses a potential issue or enhances the code's robustness, such as adding error handling.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=a0bbadc7-4bdf-4f4b-b596-33b65016d0f9&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 15
<location path="docs/release_notes.rst" line_range="37" />
<code_context>
publishing remains limited to main/release pushes. This fixes Ubuntu 26.04's
removal of the old ``qemu-user-static`` package and prevents stale registry images
-from entering a new release's platform index.
+from entering a new release's platform index. Publishing pins entries to the exact
+digests produced by pushing the tested architecture images, then verifies the
+remote index. A local-registry PR test exercises archive import, manifest-format
</code_context>
<issue_to_address>
**issue (review_instructions):** issue: “Registry publishing remains limited to main/release pushes” is too broad and conflicts with the new PR workflow.
The new ``platform-publish.yml`` workflow runs on pull requests and publishes images and an index to a local registry. Please qualify the statement to refer specifically to production/Quay publishing, or otherwise clarify the exception for local regression tests.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Identify and point out discrepancies or inaccuracies in documentation.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=09d0061e-0fcb-4f4a-a6ad-7096cf4cf877&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 16
<location path=".github/workflows/platform-publish.yml" line_range="37" />
<code_context>
+ podman push --tls-verify=false localhost/publish-source:arm64 docker://127.0.0.1:5000/fetchit-test-amd:latest
+ FETCHIT_REGISTRY_TLS_VERIFY=false bash scripts/publish-platform-index.sh \
+ 127.0.0.1:5000/fetchit-test:latest localhost/publish-source:amd64 localhost/publish-source:arm64
+ # Moving an architecture tag afterward must not change index membership.
+ podman push --tls-verify=false localhost/publish-source:arm64 docker://127.0.0.1:5000/fetchit-test-amd:latest
+ for arch in amd64 arm64; do
</code_context>
<issue_to_address>
**issue (review_instructions):** The test comment claims to verify index membership, but the following assertions only check the pulled image architectures.
Please make the test and comment unambiguous by inspecting the remote index after moving the architecture tag and asserting that it still contains exactly the expected amd64 and arm64 digests; for example, compare `podman manifest inspect` output against the digests published before the tag move.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Verify that the comment addresses potential ambiguities or misunderstandings in the code or documentation, and includes specific suggestions for changes.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=7e710e50-7c98-4c6c-84b4-935be462fec6&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 17
<location path="scripts/publish-platform-index.sh" line_range="38" />
<code_context>
+ manifests = json.load(source)['manifests']
+expected = {('linux', 'amd64'): sys.argv[2], ('linux', 'arm64'): sys.argv[3]}
+actual = {(m['platform']['os'], m['platform']['architecture']): m['digest'] for m in manifests}
+assert len(manifests) == 2 and actual == expected, (actual, expected)
+PYTHON
</code_context>
<issue_to_address>
**issue (review_instructions):** Use an explicit exception instead of assert so index verification cannot be disabled by Python optimization.
Running Python with optimization enabled skips `assert` statements, allowing an incorrect remote index to pass verification. For example, replace this with `if len(manifests) != 2 or actual != expected: raise ValueError(f"Unexpected platform index: {actual!r}; expected {expected!r}")`. No additional import is required; this also gives the caller a specific `ValueError` rather than relying on assertion behavior.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Ensure the comment provides a specific suggestion that addresses a potential bug risk; suggest using more specific exception types for better error handling; include examples or suggested implementations; mention any additional imports or changes needed.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=d27a26d6-5526-46cf-89ec-d785396d7c61&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 18
<location path="scripts/publish-platform-index.sh" line_range="5" />
<code_context>
+# Publish only the supplied, tested images; pin index entries to the pushed digests.
+set -euo pipefail
+[[ $# -eq 3 ]] || { echo 'Usage: publish-platform-index.sh DESTINATION:latest AMD64_IMAGE ARM64_IMAGE' >&2; exit 1; }
+destination=$1 amd_image=$2 arm_image=$3
+[[ "$destination" == *:latest ]] || { echo 'Destination must include :latest' >&2; exit 1; }
+repository=${destination%:latest}
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Split the positional-argument assignments and validate each image argument explicitly.
For example:
```bash
destination=$1
amd_image=$2
arm_image=$3
for image in "$amd_image" "$arm_image"; do
podman image inspect "$image" >/dev/null || { echo "Image not found: $image" >&2; exit 1; }
done
```
Separate assignments and early validation make failures clearer and reduce cognitive load when this publishing script is changed or debugged.
<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 its 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 19
<location path="scripts/publish-platform-index.sh" line_range="13" />
<code_context>
+if [[ -n ${FETCHIT_PUBLISH_AUTH_FILE:-} ]]; then options+=(--authfile="$FETCHIT_PUBLISH_AUTH_FILE"); fi
+scratch=$(mktemp -d)
+manifest="localhost/fetchit-publish-$$"
+trap 'publish_status=$?; podman manifest rm "$manifest" >/dev/null 2>&1 || true; rm -rf "$scratch"; exit "$publish_status"' EXIT
+
+test "$(podman image inspect --format '{{.Architecture}}' "$amd_image")" = amd64
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Extract the EXIT cleanup trap into a named helper to avoid dense, hard-to-maintain shell logic.
This trap combines status preservation, best-effort manifest cleanup, temporary-directory removal, and status propagation in one quoted command. Define a helper such as `cleanup() { local status=$?; podman manifest rm "$manifest" >/dev/null 2>&1 || true; rm -rf "$scratch"; return "$status"; }` and register it with `trap cleanup EXIT`. This makes the cleanup responsibilities explicit and gives future changes a safer place to be added or tested.
<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.
[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 20
<location path="scripts/publish-platform-index.sh" line_range="8" />
<code_context>
+destination=$1 amd_image=$2 arm_image=$3
+[[ "$destination" == *:latest ]] || { echo 'Destination must include :latest' >&2; exit 1; }
+repository=${destination%:latest}
+case "${FETCHIT_REGISTRY_TLS_VERIFY:-true}" in true|false) ;; *) echo 'Invalid TLS verification setting' >&2; exit 1;; esac
+options=(--tls-verify="${FETCHIT_REGISTRY_TLS_VERIFY:-true}")
+if [[ -n ${FETCHIT_PUBLISH_AUTH_FILE:-} ]]; then options+=(--authfile="$FETCHIT_PUBLISH_AUTH_FILE"); fi
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Move environment validation and Podman option construction into a helper function.
Validation, defaulting, and option construction are currently compressed into one case statement and one conditional, which makes the setup flow difficult to scan. Consider a helper such as `build_podman_options() { local verify=${FETCHIT_REGISTRY_TLS_VERIFY:-true}; [[ $verify == true || $verify == false ]] || { echo 'Invalid TLS verification setting' >&2; return 1; }; options=("--tls-verify=$verify"); [[ -z ${FETCHIT_PUBLISH_AUTH_FILE:-} ]] || options+=("--authfile=$FETCHIT_PUBLISH_AUTH_FILE"); }`, then call it once before publishing. This separates configuration parsing from the publishing workflow and uses early validation failure.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Comments should focus on reducing code complexity, such as by extracting helper functions or using early returns to flatten nested logic.
[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 21
<location path=".github/workflows/platform-publish.yml" line_range="24" />
<code_context>
+ docker.io/library/registry:2@sha256:a3d8aaa63ed8681a604f1dea0aa03f100d5895b6a58ace528858a7b332415373
+ trap 'podman logs publish-registry; podman rm -f publish-registry' EXIT
+ timeout 30 bash -c 'until curl --fail --silent http://127.0.0.1:5000/v2/; do sleep 1; done'
+ for arch in amd64 arm64; do
+ podman build --format oci --platform "linux/$arch" -t "localhost/publish-source:$arch" \
+ -f tests/integration/publishing/Containerfile tests/integration/publishing
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Extract the local-registry integration flow into a reusable test script.
This workflow step contains registry startup, cleanup, multi-architecture builds, archive round-tripping, publishing, mutation testing, and platform verification. Move the shell logic into a script such as `tests/integration/publishing/test-platform-publish.sh`, with helpers like `start_registry`, `build_archives`, `publish_index`, and `assert_platform_pull`; leave the workflow with a short invocation. For example, the workflow could run `bash tests/integration/publishing/test-platform-publish.sh`, while each helper returns early on failure and keeps the CI YAML focused on orchestration.
<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.
[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 22
<location path=".github/workflows/platform-publish.yml" line_range="1" />
<code_context>
+name: Platform publishing regression
+on:
+ pull_request:
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Clarify that this workflow is a regression test rather than a regression itself.
For example, rename it to `Platform publishing regression test` so the workflow’s purpose is immediately clear and concise.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Ensure the comment provides a clear and actionable suggestion for improvement, and includes a specific example or rephrasing.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=2bfb8466-4134-4442-a1f2-4c5bb6ccce1e&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 23
<location path=".github/workflows/platform-publish.yml" line_range="16" />
<code_context>
+ steps:
+ - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4
+ - uses: ./.github/actions/setup-podman
+ - name: Exercise archive import and complete publishing against a local registry
+ run: |
+ export XDG_RUNTIME_DIR="/run/user/$(id -u)"
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Use a more concise, concrete step name describing the actions being tested.
For example, `Test archive import and platform publishing with a local registry` is shorter and makes the regression coverage clearer than the vague phrase `complete publishing`.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Verify that the suggestion enhances clarity and conciseness of the text, with a specific example or rephrasing.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=2bfb8466-4134-4442-a1f2-4c5bb6ccce1e&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>
### Comment 24
<location path="scripts/publish-platform-index.sh" line_range="5" />
<code_context>
+# Publish only the supplied, tested images; pin index entries to the pushed digests.
+set -euo pipefail
+[[ $# -eq 3 ]] || { echo 'Usage: publish-platform-index.sh DESTINATION:latest AMD64_IMAGE ARM64_IMAGE' >&2; exit 1; }
+destination=$1 amd_image=$2 arm_image=$3
+[[ "$destination" == *:latest ]] || { echo 'Destination must include :latest' >&2; exit 1; }
+repository=${destination%:latest}
</code_context>
<issue_to_address>
**issue (review_instructions):** Validate the destination registry and image references before using the supplied auth file.
This script accepts arbitrary destination and source references while potentially loading credentials through `FETCHIT_PUBLISH_AUTH_FILE`. If an untrusted caller can control these arguments, it could redirect credentials and publish operations to an attacker-controlled registry. Add strict OCI-reference validation and, where this script is intended only for the release registry, enforce an allowlist such as `quay.io/fetchit`; also reject option-like values before invoking Podman.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `*`
**Instructions:**
Include comments that suggest security improvements, such as input validation, especially when they prevent potential runtime failures or malicious use.
[Edit this rule](https://app.sourcery.ai/accounts/150647/repo-settings/455664976?activeTab=review-rules&rule=d61aca01-181f-4b01-a1fb-8449fd0887e6&from_surface=github_bot&intent=review_rules)
</details>
</issue_to_address>| destination=$1 amd_image=$2 arm_image=$3 | ||
| [[ "$destination" == *:latest ]] || { echo 'Destination must include :latest' >&2; exit 1; } | ||
| repository=${destination%:latest} | ||
| case "${FETCHIT_REGISTRY_TLS_VERIFY:-true}" in true|false) ;; *) echo 'Invalid TLS verification setting' >&2; exit 1;; esac |
There was a problem hiding this comment.
suggestion (review_instructions): Move environment validation and Podman option construction into a helper function.
Validation, defaulting, and option construction are currently compressed into one case statement and one conditional, which makes the setup flow difficult to scan. Consider a helper such as build_podman_options() { local verify=${FETCHIT_REGISTRY_TLS_VERIFY:-true}; [[ $verify == true || $verify == false ]] || { echo 'Invalid TLS verification setting' >&2; return 1; }; options=("--tls-verify=$verify"); [[ -z ${FETCHIT_PUBLISH_AUTH_FILE:-} ]] || options+=("--authfile=$FETCHIT_PUBLISH_AUTH_FILE"); }, then call it once before publishing. This separates configuration parsing from the publishing workflow and uses early validation failure.
Review instructions:
Path patterns: *
Instructions:
Comments should focus on reducing code complexity, such as by extracting helper functions or using early returns to flatten nested logic.
| # Publish only the supplied, tested images; pin index entries to the pushed digests. | ||
| set -euo pipefail | ||
| [[ $# -eq 3 ]] || { echo 'Usage: publish-platform-index.sh DESTINATION:latest AMD64_IMAGE ARM64_IMAGE' >&2; exit 1; } | ||
| destination=$1 amd_image=$2 arm_image=$3 |
There was a problem hiding this comment.
issue (review_instructions): Validate the destination registry and image references before using the supplied auth file.
This script accepts arbitrary destination and source references while potentially loading credentials through FETCHIT_PUBLISH_AUTH_FILE. If an untrusted caller can control these arguments, it could redirect credentials and publish operations to an attacker-controlled registry. Add strict OCI-reference validation and, where this script is intended only for the release registry, enforce an allowlist such as quay.io/fetchit; also reject option-like values before invoking Podman.
Review instructions:
Path patterns: *
Instructions:
Include comments that suggest security improvements, such as input validation, especially when they prevent potential runtime failures or malicious use.
…ecks Signed-off-by: Ryan Cook <rcook@redhat.com>
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
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.
Publishing main images fails when a locally assembled index refers to manifest instances that change during individual image pushes. Build the published index from the exact digests returned by those pushes and verify the remote platform entries.
Add a credential-free local-registry Actions test covering Docker archive save/load, stale architecture tags, full index publication, and pulls for both architectures. Existing PR image assembly checks remain in place.
Validation: Bash syntax, actionlint, and diff checks pass locally; the registry workflow exercises the runtime publishing path in CI.
Summary by Sourcery
Make multi-architecture image publishing reliable by publishing tested images through a digest-pinned index workflow and exercising it against a local registry.
Bug Fixes:
Enhancements:
CI:
Documentation:
Tests: