Repository navigation
packaging(ns8): address code review - multi-arch module image and workflow hardening #137
There are no files selected for viewing
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| #!/bin/bash | ||
|
|
||
| # | ||
| # Copyright (C) 2023 Nethesis S.r.l. | ||
| # SPDX-License-Identifier: GPL-3.0-or-later | ||
| # | ||
|
|
||
| # Assemble the per-arch module images built by build-images.sh into one | ||
| # multi-arch tag. | ||
| # | ||
| # NS8 runs each module as a rootless container on the node, so the module | ||
| # image must exist for every node architecture. A single-arch image would | ||
| # install fine on amd64 and fail on arm64, so the per-arch tags are never | ||
| # published as-is: only the assembled tags are public. | ||
| # | ||
| # Run from this directory (packaging/ns8), after pushing each arch with | ||
| # build-images.sh. Environment: | ||
| # REPOBASE module image namespace, default ghcr.io/calnode | ||
| # TARGETARCHES space-separated arches to assemble, required | ||
| # IMAGETAGS space-separated tags to publish, default "latest" | ||
| # | ||
| # Example: | ||
| # TARGETARCHES="amd64 arm64" IMAGETAGS="1.2.3 1.2 latest" bash push-manifest.sh | ||
|
|
||
| set -e | ||
|
|
||
| repobase="${REPOBASE:-ghcr.io/calnode}" | ||
| repobase="${repobase,,}" | ||
| reponame="ns8-calnode" | ||
| read -r -a arches <<< "${TARGETARCHES:-}" | ||
| read -r -a imagetags <<< "${IMAGETAGS:-latest}" | ||
|
|
||
| if [ "${#arches[@]}" -eq 0 ]; then | ||
| echo "TARGETARCHES is required, e.g. TARGETARCHES=\"amd64 arm64\"" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| for arch in "${arches[@]}"; do | ||
| if ! buildah image exists "${repobase}/${reponame}:build-${arch}"; then | ||
| echo "Missing local image for ${arch}. Run build-images.sh with" \ | ||
| "TARGETARCH=${arch} PUSH=1 first." >&2 | ||
| exit 1 | ||
| fi | ||
| done | ||
|
|
||
| for imagetag in "${imagetags[@]}"; do | ||
| manifest="${repobase}/${reponame}:${imagetag}" | ||
| # Drop any stale manifest so a retry cannot inherit old entries. | ||
| buildah manifest rm --all "${manifest}" 2>/dev/null || true | ||
|
Contributor
There was a problem hiding this comment.
Technical details# `buildah manifest rm --all` is not a valid invocation
## Affected sites
- `packaging/ns8/push-manifest.sh:49` — `buildah manifest rm --all "${manifest}" 2>/dev/null || true`
## Required outcome
- Remove any existing local manifest list named `${manifest}` before recreating it, without erroring when none exists.
## Suggested approach (optional)
- `buildah manifest rm "${manifest}" 2>/dev/null || true`.
- `manifest rm` has no flags registered; only the `rm`/`remove` positional list names are accepted. |
||
| buildah manifest create "${manifest}" | ||
| for arch in "${arches[@]}"; do | ||
| buildah manifest add \ | ||
| "${manifest}" \ | ||
| "${repobase}/${reponame}:build-${arch}" \ | ||
| "docker.io/calnode/ns8-calnode:${imagetag}-${arch}" | ||
|
Comment on lines
+52
to
+55
Contributor
There was a problem hiding this comment.
The image's platform is already taken from its config, and Technical details# Too many arguments to `buildah manifest add`
## Affected sites
- `packaging/ns8/push-manifest.sh:52-55` — `buildah manifest add "${manifest}" "${repobase}/${reponame}:build-${arch}" "docker.io/calnode/ns8-calnode:${imagetag}-${arch}"`
## Required outcome
- Add exactly the local per-arch image to the manifest list and let its recorded platform come from the image config.
## Suggested approach (optional)
- Drop the third argument entirely, i.e. `buildah manifest add "${manifest}" "${repobase}/${reponame}:build-${arch}"`.
- If you want the platform to be explicit rather than inferred, add `--arch "${arch}"`.
- Note the dropped value also hardcodes `docker.io/calnode/ns8-calnode`, which diverges from `${repobase}/${reponame}` (e.g. a fork's owner) — another reason to remove it rather than repoint it.
## Verified
- `buildah manifest add` non-artifact path rejects `len(args) > 2`; checked in both buildah `main` and `v1.33.7` (Ubuntu 24.04 runner).
- `buildah from --arch arm64 scratch` + `buildah commit` records `Architecture=arm64` in the config (`fixupConfig` uses `systemContext.ArchitectureChoice`), so `manifest add` reads the right platform without `--arch`. |
||
| done | ||
| buildah manifest push --all "${manifest}" "docker://${manifest}" | ||
| buildah manifest rm "${manifest}" | ||
| echo "Published ${manifest} (${arches[*]})" | ||
| done | ||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
buildahhas noimagesubcommand — it isbuildah images(list) andbuildah manifest exists(checks a manifest list).buildah image exists …fails with an unknown-command error, and because the test isif ! buildah image exists …, that failure is read as "image missing", so the loop always printsMissing local image …and the script exits 1 before assembling anything.Technical details