COPR #107
COPR #107
Conversation
WalkthroughAdds COPR-based RPM build flow and configurable builder selection; new COPR builder/containerfiles and scripts; specfile restructures for kindnet/topolvm; CI/workflow changes to pass rpm-builder, derive VERSION, and expand per-container sosreport collection; docs updated for COPR/local RPM install paths. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Prebuild as Prebuild Action
participant Make as Make / copr-rpm
participant CoprCli as copr-cli container
participant RPMS as /mnt/rpms
participant Bootc as build action (bootc)
GH->>Prebuild: Prepare build environment (local prebuild action)
alt Build RPMs to COPR
Prebuild->>Make: run copr-rpm (podman builder + secret)
Make->>CoprCli: submit SRPMs -> returns build IDs
CoprCli-->>Make: wait, download RPMs -> write /mnt/rpms/{version.txt,builds.txt}
end
GH->>GH: read VERSION from /mnt/rpms/version.txt
GH->>Bootc: invoke build action with rpm-builder (passes BUILDER_IMAGE -> RPM_BUILDER_IMAGE)
Bootc-->>GH: bootc image built & tagged using VERSION
alt On failure/cleanup
GH->>CoprCli: copr-delete-builds / copr-regenerate-repos (use COPR_CONFIG/secret)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas needing extra attention:
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-10-17T10:53:08.461ZApplied to files:
📚 Learning: 2025-10-17T07:44:32.742ZApplied to files:
📚 Learning: 2025-10-17T10:31:57.408ZApplied to files:
🔇 Additional comments (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
pmtk
force-pushed
the
copr
branch
16 times, most recently
from
e60df4b to
d179612
Compare
Most important: - MICROSHIFT_VERSION is $USHIFT_BRANCH + $OKD_VERSION_TAG - SOURCE_GIT_TREE_STATE is now hardcoded to 'clean' because we always change the specfile which always result in 'dirty' - SOURCE_GIT_TAG and SOURCE_GIT_COMMIT copied from Makefile libraries
pmtk
force-pushed
the
copr
branch
2 times, most recently
from
5cbccfb to
f837481
Compare
pmtk
force-pushed
the
copr
branch
5 times, most recently
from
1a599c0 to
47cfb84
Compare
43 was released recently and there are build problems
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/image/modify-spec.py (1)
79-79: Critical: Wrong variable name - usecontainerinstead ofop.Line 79 uses undefined variable
opinstead ofcontainer, which will either cause a NameError or produce incorrect results ifopis defined in an outer scope.Apply this diff:
- new_image=$(jq -r --arg NAME "${op}" '.references.spec.tags[] | select(.name == $NAME) | .from.name' "${okd_json}" || true) + new_image=$(jq -r --arg NAME "${container}" '.references.spec.tags[] | select(.name == $NAME) | .from.name' "${okd_json}" || true)
🧹 Nitpick comments (2)
src/copr/create-builds-and-wait.sh (1)
15-15: Hardcoded chroot limits multi-architecture and multi-distribution support.The script only downloads RPMs for
centos-stream-9-x86_64, which prevents usage with other architectures (aarch64) or distributions (Fedora). Consider making this configurable via an environment variable.+COPR_CHROOT="${COPR_CHROOT:-centos-stream-9-x86_64}" + for b in $builds ; do - copr download-build --rpms --chroot centos-stream-9-x86_64 --dest ./rpms "${b}"; + copr download-build --rpms --chroot "${COPR_CHROOT}" --dest ./rpms "${b}"; doneThen update line 22 accordingly:
-cp -v ./rpms/centos-stream-9-x86_64/*.rpm "${HOME}/microshift/_output/rpmbuild/RPMS/" +cp -v ./rpms/${COPR_CHROOT}/*.rpm "${HOME}/microshift/_output/rpmbuild/RPMS/"src/image/modify-spec.py (1)
46-46: Remove unused exception variable.The exception is caught but never used.
Apply this diff:
- except ValueError as e: + except ValueError: pass
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.github/actions/build/action.yaml(4 hunks).github/workflows/builders.yaml(1 hunks).github/workflows/release.md(1 hunks).github/workflows/release.yaml(3 hunks)Makefile(5 hunks)docs/run.md(3 hunks)packaging/bootc.Containerfile(1 hunks)packaging/rpm-copr-builder.Containerfile(1 hunks)packaging/rpm-local-builder.Containerfile(3 hunks)src/copr/copr-cli.Containerfile(1 hunks)src/copr/copr.mk(1 hunks)src/copr/create-builds-and-wait.sh(1 hunks)src/image/build-rpms.sh(1 hunks)src/image/modify-spec.py(1 hunks)src/image/prebuild.sh(3 hunks)src/kindnet/kindnet.spec(3 hunks)src/topolvm/topolvm.spec(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-17T07:44:32.742Z
Learnt from: ggiguash
Repo: microshift-io/microshift PR: 92
File: .github/workflows/release.yaml:44-50
Timestamp: 2025-10-17T07:44:32.742Z
Learning: When Podman builds an image without a registry prefix (e.g., `podman build -t microshift-okd`), it automatically adds the `localhost/` prefix and `:latest` tag, resulting in `localhost/microshift-okd:latest`. This means the Makefile in microshift-io/microshift building with `-t microshift-okd` produces `localhost/microshift-okd:latest` without explicit retagging.
Applied to files:
packaging/bootc.Containerfilesrc/image/build-rpms.shpackaging/rpm-local-builder.Containerfilesrc/copr/copr.mksrc/image/prebuild.sh.github/workflows/builders.yaml.github/actions/build/action.yaml.github/workflows/release.yamlpackaging/rpm-copr-builder.ContainerfileMakefile
📚 Learning: 2025-10-17T10:53:08.461Z
Learnt from: ggiguash
Repo: microshift-io/microshift PR: 57
File: .github/workflows/builders.yaml:75-84
Timestamp: 2025-10-17T10:53:08.461Z
Learning: In the microshift repository, the RPM-to-Debian conversion workflow follows this pattern: `convert.sh` receives an RPM directory as input and outputs .deb files to a `deb/` subdirectory within that directory. The `install.sh` script expects to receive the parent RPM directory (not the deb subdirectory) and uses recursive `find` commands to locate the .deb files in subdirectories.
Applied to files:
src/image/build-rpms.shpackaging/rpm-local-builder.Containerfile.github/workflows/release.mdsrc/copr/copr.mksrc/image/prebuild.shsrc/copr/create-builds-and-wait.sh.github/actions/build/action.yaml.github/workflows/release.yamlpackaging/rpm-copr-builder.Containerfiledocs/run.mdMakefilesrc/topolvm/topolvm.spec
📚 Learning: 2025-10-17T10:31:57.408Z
Learnt from: ggiguash
Repo: microshift-io/microshift PR: 57
File: src/debian/install.sh:12-0
Timestamp: 2025-10-17T10:31:57.408Z
Learning: In the MicroShift project, the Ubuntu version in src/debian/install.sh is intentionally hardcoded to "xUbuntu_20.04" because CRI-O builds are not available for all the latest Ubuntu versions. This should not be changed to dynamic detection.
Applied to files:
src/image/build-rpms.shpackaging/rpm-local-builder.Containerfilesrc/image/prebuild.shsrc/topolvm/topolvm.spec
📚 Learning: 2025-10-29T07:57:10.203Z
Learnt from: pacevedom
Repo: microshift-io/microshift PR: 103
File: src/cluster_manager.sh:327-327
Timestamp: 2025-10-29T07:57:10.203Z
Learning: In src/cluster_manager.sh, the file `/var/lib/microshift/resources/kubeadmin/kubeconfig` (without a node name) is guaranteed to exist if MicroShift has started and is ready, as it is used internally. This generic kubeconfig path is appropriate for cluster-wide operations like querying cluster status, while node-specific paths like `/var/lib/microshift/resources/kubeadmin/${node_name}/kubeconfig` are used for node-specific operations like joining nodes.
Applied to files:
docs/run.md
🪛 GitHub Actions: builders
Makefile
[error] 149-149: make run-healthy failed. Node microshift-okd-1 is not healthy after health checks.
🪛 LanguageTool
docs/run.md
[grammar] ~23-~23: Use a hyphen to join words.
Context: ...://okd.io/docs/operators/) | ## Package based systems (non-bootc) ### Install M...
(QB_NEW_EN_HYPHEN)
🪛 Ruff (0.14.3)
src/image/modify-spec.py
1-1: Shebang is present but file is not executable
(EXE001)
46-46: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🔇 Additional comments (5)
.github/workflows/builders.yaml (1)
64-64: LGTM - Version pinning improves reproducibility.Pinning to Fedora 42 instead of
latestmakes the workflow more deterministic and prevents unexpected breaks from upstream changes.packaging/bootc.Containerfile (1)
5-7: LGTM - Clean parameterization of builder image.The configurable
RPM_BUILDER_IMAGEenables switching between local and COPR builders while maintaining backward compatibility with the default.src/copr/copr-cli.Containerfile (1)
1-3: LGTM - Minimal and focused container definition.The container correctly installs copr-cli and cleans up to minimize image size.
.github/actions/build/action.yaml (1)
46-50: LGTM - Clean parameterization of RPM builder.The new input enables selection between local and COPR builders with a sensible default.
src/image/prebuild.sh (1)
23-36: LGTM - Optimization reduces redundant API calls.Caching the OKD release JSON and reusing it eliminates multiple expensive
oc adm release infocalls, significantly improving performance.
| Enable COPR repository (optionally specify chroot such as `centos-stream-9-{x86_64,aarch64}`, `fedora-42-{x86_64,aarch64}`): | ||
| ```sh | ||
| sudo dnf copr enable $COPR_REPO_NAME [chroot] | ||
| ``` | ||
|
|
||
| Next, install MicroShift: | ||
| ```sh | ||
| sudo dnf install -y \ | ||
| microshift-$VERSION \ | ||
| microshift-kindnet-$VERSION \ | ||
| microshift-topolvm-$VERSION | ||
| ``` |
There was a problem hiding this comment.
Clarify where $VERSION and $COPR_REPO_NAME variables are defined.
The documentation uses $VERSION and $COPR_REPO_NAME variables without explaining where they come from or how users should set them. Consider adding a note at the beginning explaining these are placeholders that should be replaced with actual values (e.g., export VERSION=4.18.0 and export COPR_REPO_NAME=microshift-io/microshift).
🤖 Prompt for AI Agents
In .github/workflows/release.md around lines 5 to 16, the instructions use the
placeholders $VERSION and $COPR_REPO_NAME without defining them; update the doc
to state these are user-defined environment variables and show how to set them
before running the commands (for example, indicate users should export VERSION
and COPR_REPO_NAME with the desired values or replace the placeholders inline),
and add a brief example line or sentence near the top of this section that
demonstrates setting these variables (e.g., export VERSION=4.18.0; export
COPR_REPO_NAME=microshift-io/microshift) so readers know where the values come
from.
| copr-repo: | ||
| default: pmtk0/test123 | ||
| description: COPR repository name | ||
| type: string |
There was a problem hiding this comment.
Don't default to a personal COPR repo.
Leaving copr-repo defaulted to pmtk0/test123 means anyone running the release workflow without overriding the input will publish the RPMs into that test project. Please drop the default (or point it at the official MicroShift COPR namespace) so releases can't accidentally land in the wrong repo.
🤖 Prompt for AI Agents
In .github/workflows/release.yaml around lines 15 to 18, the workflow input
`copr-repo` is defaulting to a personal COPR repo (`pmtk0/test123`); remove that
default value (or replace it with the official MicroShift COPR namespace such as
`openshift/microshift`) so running the workflow without an explicit input cannot
publish to a personal test project; keep the input name, description and type,
and update any documentation/comments to note that callers must supply the
intended COPR repo when invoking the workflow.
| sudo podman build \ | ||
| --tag "${COPR_BUILDER_IMAGE}" \ | ||
| --secret id=${COPR_SECRET_NAME},src=${COPR_CONFIG} \ | ||
| --ulimit nofile=524288:524288 \ | ||
| --build-arg USHIFT_BRANCH="${USHIFT_BRANCH}" \ | ||
| --build-arg OKD_VERSION_TAG="${OKD_VERSION_TAG}" \ | ||
| --env COPR_REPO_NAME="${COPR_REPO_NAME}" \ | ||
| --file packaging/rpm-copr-builder.Containerfile . |
There was a problem hiding this comment.
Fix podman build invocation
podman build doesn’t support --env, so this command fails and copr-rpm never reaches the container build. Please swap to a build arg (and have the Containerfile accept it) so we can override COPR_REPO_NAME without breaking the build.
- --env COPR_REPO_NAME="${COPR_REPO_NAME}" \
+ --build-arg COPR_REPO_NAME="${COPR_REPO_NAME}" \You’ll also need to introduce ARG COPR_REPO_NAME and wire it into ENV COPR_REPO_NAME inside packaging/rpm-copr-builder.Containerfile.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/copr/copr.mk around lines 13 to 20, the podman build invocation
incorrectly uses --env which podman build does not support; replace --env
COPR_REPO_NAME="${COPR_REPO_NAME}" with --build-arg
COPR_REPO_NAME="${COPR_REPO_NAME}" in the command, and update
packaging/rpm-copr-builder.Containerfile to declare ARG COPR_REPO_NAME and set
ENV COPR_REPO_NAME=${COPR_REPO_NAME} so the build arg is accepted and exported
into the image.
|
|
||
| SRPMS="${HOME}/microshift/_output/rpmbuild/SRPMS" | ||
|
|
||
| out="$(copr-cli --config /run/secrets/copr-cfg build --nowait "${COPR_REPO_NAME}" "${SRPMS}"/microshift*.src.rpm)" |
There was a problem hiding this comment.
Validate COPR_REPO_NAME environment variable.
The script uses $COPR_REPO_NAME without checking if it's set, which would cause a cryptic error from copr-cli.
Apply this diff to add validation:
+if [ -z "${COPR_REPO_NAME:-}" ]; then
+ echo "ERROR: COPR_REPO_NAME environment variable is not set"
+ exit 1
+fi
+
out="$(copr-cli --config /run/secrets/copr-cfg build --nowait "${COPR_REPO_NAME}" "${SRPMS}"/microshift*.src.rpm)"🤖 Prompt for AI Agents
In src/copr/create-builds-and-wait.sh around line 6, the script uses
$COPR_REPO_NAME without validating it; add a guard before the copr-cli call that
checks COPR_REPO_NAME is non-empty (e.g., if [ -z "${COPR_REPO_NAME}" ]), print
a clear error to stderr indicating the variable is required, and exit non-zero;
then proceed to run the existing copr-cli command using the validated and
properly quoted ${COPR_REPO_NAME}.
|
|
||
| builds=$(echo "${out}" | grep "Created builds" | cut -d: -f2 | xargs) | ||
| # shellcheck disable=SC2086 | ||
| copr-cli watch-build ${builds} |
There was a problem hiding this comment.
Inconsistent copr-cli invocation - missing --config flag.
Line 6 uses --config /run/secrets/copr-cfg but this line omits it, which may cause the command to use the default config location instead.
Apply this diff:
-copr-cli watch-build ${builds}
+copr-cli --config /run/secrets/copr-cfg watch-build ${builds}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| copr-cli watch-build ${builds} | |
| copr-cli --config /run/secrets/copr-cfg watch-build ${builds} |
🤖 Prompt for AI Agents
In src/copr/create-builds-and-wait.sh around line 11, the copr-cli watch-build
invocation omits the --config flag used elsewhere; update the command to include
--config /run/secrets/copr-cfg (e.g., add the flag before the ${builds}
argument) so it uses the intended config file instead of the default.
| @@ -0,0 +1,69 @@ | |||
| #!/usr/bin/env python | |||
There was a problem hiding this comment.
Make script executable.
The shebang is present but the file lacks execute permission.
Run: chmod +x src/image/modify-spec.py
🧰 Tools
🪛 Ruff (0.14.3)
1-1: Shebang is present but file is not executable
(EXE001)
🤖 Prompt for AI Agents
In src/image/modify-spec.py around line 1: the file has a shebang but lacks
executable permission; make the script executable by running chmod +x
src/image/modify-spec.py (or set executable bit in git index/CI) so it can be
run directly from the shell.
| # Get the new image from OKD release | ||
| local new_image | ||
| new_image=$(oc adm release info --image-for="${container}" "${okd_url}:${okd_releaseTag}" || true) | ||
| new_image=$(jq -r --arg NAME "${op}" '.references.spec.tags[] | select(.name == $NAME) | .from.name' "${okd_json}" || true) |
There was a problem hiding this comment.
Critical: Wrong variable name - use container instead of op.
Line 79 uses undefined variable op instead of container, causing incorrect OLM image lookup. This is the same bug present in src/image/modify-spec.py:79.
Apply this diff:
- new_image=$(jq -r --arg NAME "${op}" '.references.spec.tags[] | select(.name == $NAME) | .from.name' "${okd_json}" || true)
+ new_image=$(jq -r --arg NAME "${container}" '.references.spec.tags[] | select(.name == $NAME) | .from.name' "${okd_json}" || true)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| new_image=$(jq -r --arg NAME "${op}" '.references.spec.tags[] | select(.name == $NAME) | .from.name' "${okd_json}" || true) | |
| new_image=$(jq -r --arg NAME "${container}" '.references.spec.tags[] | select(.name == $NAME) | .from.name' "${okd_json}" || true) |
🤖 Prompt for AI Agents
In src/image/prebuild.sh around line 79, the jq command uses an undefined
variable `op`, causing incorrect OLM image lookup; change the shell variable
reference from `"$op"` to `"$container"` so jq selects the tag by the actual
container name (i.e., replace --arg NAME "${op}" with --arg NAME "${container}"
and keep the rest of the command intact). Ensure you update the corresponding
similar occurrence in src/image/modify-spec.py:79 as well.
1b7a0ba to
51573d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/actions/build/action.yaml(4 hunks)packaging/rpm-local-builder.Containerfile(2 hunks)src/image/postbuild.sh(0 hunks)
💤 Files with no reviewable changes (1)
- src/image/postbuild.sh
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-10-17T10:53:08.461Z
Learnt from: ggiguash
Repo: microshift-io/microshift PR: 57
File: .github/workflows/builders.yaml:75-84
Timestamp: 2025-10-17T10:53:08.461Z
Learning: In the microshift repository, the RPM-to-Debian conversion workflow follows this pattern: `convert.sh` receives an RPM directory as input and outputs .deb files to a `deb/` subdirectory within that directory. The `install.sh` script expects to receive the parent RPM directory (not the deb subdirectory) and uses recursive `find` commands to locate the .deb files in subdirectories.
Applied to files:
.github/actions/build/action.yamlpackaging/rpm-local-builder.Containerfile
📚 Learning: 2025-10-17T07:44:32.742Z
Learnt from: ggiguash
Repo: microshift-io/microshift PR: 92
File: .github/workflows/release.yaml:44-50
Timestamp: 2025-10-17T07:44:32.742Z
Learning: When Podman builds an image without a registry prefix (e.g., `podman build -t microshift-okd`), it automatically adds the `localhost/` prefix and `:latest` tag, resulting in `localhost/microshift-okd:latest`. This means the Makefile in microshift-io/microshift building with `-t microshift-okd` produces `localhost/microshift-okd:latest` without explicit retagging.
Applied to files:
.github/actions/build/action.yamlpackaging/rpm-local-builder.Containerfile
📚 Learning: 2025-10-17T10:31:57.408Z
Learnt from: ggiguash
Repo: microshift-io/microshift PR: 57
File: src/debian/install.sh:12-0
Timestamp: 2025-10-17T10:31:57.408Z
Learning: In the MicroShift project, the Ubuntu version in src/debian/install.sh is intentionally hardcoded to "xUbuntu_20.04" because CRI-O builds are not available for all the latest Ubuntu versions. This should not be changed to dynamic detection.
Applied to files:
packaging/rpm-local-builder.Containerfile
🪛 GitHub Actions: linters
packaging/rpm-local-builder.Containerfile
[warning] 49-49: ShellCheck SC2086: Double quote to prevent globbing and word splitting.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: isolated-network-kindnet
- GitHub Check: isolated-network-ovnk
- GitHub Check: fedora-bootc
- GitHub Check: ubuntu-rpm2deb
- GitHub Check: centos10-bootc
- GitHub Check: centos9-bootc
🔇 Additional comments (6)
packaging/rpm-local-builder.Containerfile (3)
15-15: LGTM - Centralized build script parameter added.The new ARG properly parameterizes the RPM build script path for the centralized workflow.
63-68: All TopoLVM source paths verified to exist.The COPY commands reference valid paths—
topolvm.spec,assets/,dropins/,greenboot/, andrelease/all exist insrc/topolvm/.
54-60: No issues found.All Kindnet asset paths in the COPY commands (lines 54-57) are confirmed to exist:
src/kindnet/kindnet.specsrc/kindnet/assets/src/kindnet/dropins/src/kindnet/crio.conf.d/The file structure is valid and the Dockerfile references are correct.
.github/actions/build/action.yaml (3)
46-50: LGTM. Clean input definition with sensible default.
64-65: LGTM. Conditional logic correctly gates local build steps based on builder type.Also applies to: 69-69
100-100: Verify BUILDER_IMAGE resolution in Makefile. Line 100 passesinputs.rpm-builder(which defaults to the stringrpm-local-builder) asBUILDER_IMAGE. Confirm that the Makefile correctly resolves this to a valid container image reference (e.g., does it constructlocalhost/rpm-local-builder:latestor expect the full reference?).
.github/actions/build/action.yaml
Outdated
| for i in $(seq 2 ${{ inputs.node-count }}); do | ||
| # Check if the MicroShift container is running | ||
| if ! sudo podman ps --format "{{.Names}}" | grep -q "^microshift-okd-${i}$" ; then | ||
| echo "WARNING: MicroShift container number ${i} is not running - cannot collect sos report" | ||
| exit 0 | ||
| fi | ||
| # Collect sos report from the MicroShift container | ||
| sudo podman exec -i "microshift-okd-${i}" microshift-sos-report | ||
| for f in $(sudo podman exec -i "microshift-okd-${i}" bash -c 'ls -1 /tmp/sosreport-*'); do | ||
| sudo podman cp "microshift-okd-${i}":${f} /mnt/tmp/ | ||
| sudo chmod 644 "/mnt/tmp/$(basename "${f}")" | ||
| done | ||
| done |
There was a problem hiding this comment.
sosreport loop skips the main container. Line 157 starts the loop at seq 2, which means only microshift-okd-2 through microshift-okd-${node-count} are processed. The main microshift-okd container (created at line 115 by make run) is never included. This issue was flagged in a prior review and remains unresolved. For a single-node cluster (node-count=1), no sosreports are collected at all.
Collect from the main container first, then loop over additional nodes:
- name: Collect MicroShift container sosreport on failure
if: failure()
shell: bash
run: |
set -euo pipefail
set -x
sudo podman ps --all
+ # Collect sosreport from the main MicroShift container
+ if sudo podman ps --format "{{.Names}}" | grep -q "^microshift-okd$" ; then
+ sudo podman exec -i microshift-okd microshift-sos-report
+ for f in $(sudo podman exec -i microshift-okd bash -c 'ls -1 /tmp/sosreport-*'); do
+ sudo podman cp microshift-okd:${f} /mnt/tmp/
+ sudo chmod 644 "/mnt/tmp/$(basename "${f}")"
+ done
+ fi
+
for i in $(seq 2 ${{ inputs.node-count }}); do
# Check if the MicroShift container is running
if ! sudo podman ps --format "{{.Names}}" | grep -q "^microshift-okd-${i}$" ; then
echo "WARNING: MicroShift container number ${i} is not running - cannot collect sos report"
exit 0
fi🤖 Prompt for AI Agents
.github/actions/build/action.yaml around lines 157 to 170: the current loop
starts at seq 2 and therefore never collects sosreport from the main
microshift-okd container (and collects nothing when node-count=1); modify the
logic to first collect from the main container "microshift-okd" (run the same
microshift-sos-report, copy files to /mnt/tmp and set permissions) and then run
the existing loop for additional nodes using seq 2 ${{ inputs.node-count }} so
the main node is always processed and multi-node behavior remains unchanged.
| COPY --chmod=755 ./src/image/build-rpms.sh ${USHIFT_BUILDRPMS_SCRIPT} | ||
| COPY --chmod=755 ./src/image/modify-spec.py ${USHIFT_MODIFY_SPEC_SCRIPT} |
There was a problem hiding this comment.
Critical: Missing ARG declaration for USHIFT_MODIFY_SPEC_SCRIPT.
Line 45 references ${USHIFT_MODIFY_SPEC_SCRIPT}, but this ARG is never declared. The variable will expand to an empty string, causing the COPY command to fail.
Apply this diff to declare the missing ARG:
ARG USHIFT_POSTBUILD_SCRIPT=/tmp/postbuild.sh
ARG USHIFT_BUILDRPMS_SCRIPT=/tmp/build-rpms.sh
+ARG USHIFT_MODIFY_SPEC_SCRIPT=/tmp/modify-spec.pyCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packaging/rpm-local-builder.Containerfile around lines 44 to 45, the COPY
references ${USHIFT_MODIFY_SPEC_SCRIPT} which is not declared; add an ARG
declaration for USHIFT_MODIFY_SPEC_SCRIPT (with the intended default path used
in the image) above these COPY lines so the variable expands correctly at build
time; ensure the ARG name exactly matches USHIFT_MODIFY_SPEC_SCRIPT and is
declared before any instructions that use it.
| RUN sed -i -e 's,CHECK_RPMS="y",,g' -e 's,CHECK_SRPMS="y",,g' ./packaging/rpm/make-rpm.sh && \ | ||
| python3 ${USHIFT_MODIFY_SPEC_SCRIPT} && \ | ||
| "${USHIFT_BUILDRPMS_SCRIPT}" |
There was a problem hiding this comment.
Fix ShellCheck warning: quote the modify-spec script variable.
Line 49 triggers SC2086 due to unquoted variable in sed. Additionally, line 50 should quote ${USHIFT_MODIFY_SPEC_SCRIPT} to prevent word splitting.
Apply this diff:
-RUN sed -i -e 's,CHECK_RPMS="y",,g' -e 's,CHECK_SRPMS="y",,g' ./packaging/rpm/make-rpm.sh && \
- python3 ${USHIFT_MODIFY_SPEC_SCRIPT} && \
+RUN sed -i -e 's,CHECK_RPMS="y",,g' -e 's,CHECK_SRPMS="y",,g' ./packaging/rpm/make-rpm.sh && \
+ python3 "${USHIFT_MODIFY_SPEC_SCRIPT}" && \
"${USHIFT_BUILDRPMS_SCRIPT}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| RUN sed -i -e 's,CHECK_RPMS="y",,g' -e 's,CHECK_SRPMS="y",,g' ./packaging/rpm/make-rpm.sh && \ | |
| python3 ${USHIFT_MODIFY_SPEC_SCRIPT} && \ | |
| "${USHIFT_BUILDRPMS_SCRIPT}" | |
| RUN sed -i -e 's,CHECK_RPMS="y",,g' -e 's,CHECK_SRPMS="y",,g' ./packaging/rpm/make-rpm.sh && \ | |
| python3 "${USHIFT_MODIFY_SPEC_SCRIPT}" && \ | |
| "${USHIFT_BUILDRPMS_SCRIPT}" |
🧰 Tools
🪛 GitHub Actions: linters
[warning] 49-49: ShellCheck SC2086: Double quote to prevent globbing and word splitting.
🤖 Prompt for AI Agents
In packaging/rpm-local-builder.Containerfile around lines 49 to 51, the
shellcheck warning is caused by unquoted variable expansions leading to
potential word-splitting; quote the script variables and any file paths: change
${USHIFT_MODIFY_SPEC_SCRIPT} to "${USHIFT_MODIFY_SPEC_SCRIPT}" and
"${USHIFT_BUILDRPMS_SCRIPT}" similarly, and ensure the sed target path is quoted
if it may contain spaces (e.g. "./packaging/rpm/make-rpm.sh"); this removes
SC2086 and prevents unwanted word-splitting during RUN.
|
o/ Didn't read much of the code, honestly, but testing this, the COPR_REPO_NAME seems not to be passed properly. |
2 participants
Summary by CodeRabbit
New Features
Documentation
Chores