microshift-io/microshift-nightly COPR: RPM with rhocp-mirror-beta configs#203
Open
pmtk wants to merge 5 commits intomicroshift-io:mainfrom
Open
microshift-io/microshift-nightly COPR: RPM with rhocp-mirror-beta configs#203pmtk wants to merge 5 commits intomicroshift-io:mainfrom
pmtk wants to merge 5 commits intomicroshift-io:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughParameterizes the nightly COPR workflow with a COPR repo input, adds jobs and scripts to build/publish MicroShift dependencies and CNI plugins to COPR, updates COPR CLI container and Makefile targets to use a mounted secret, and changes installer docs/scripts to install a microshift-io-dependencies package instead of creating local COPR repos. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Repo as Upstream Repo
participant Detect as detect-okd-version Action
participant CLI as COPR CLI Container
participant COPR as COPR Service
GH->>Repo: checkout upstream
GH->>Detect: run to detect OKD tag
Detect-->>GH: OKD tag
GH->>CLI: invoke `copr-dependencies` (mount secret at target path)
CLI->>COPR: query package existence
alt package missing
CLI->>COPR: upload SRPM & create build
COPR-->>CLI: build status (watch)
CLI->>COPR: regenerate repos
else package exists
COPR-->>CLI: indicate existing build
end
GH->>CLI: invoke `copr-cni` (mount secret)
CLI->>COPR: upload CNI SRPM & create build
COPR-->>CLI: build status (watch)
CLI->>COPR: regenerate repos
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@docs/run.md`:
- Around line 44-47: Replace the incorrect package name
"microshift-io-dependencies-repo" in the installation command with the correct
package "microshift-io-dependencies" so the docs match the spec and quickrpm.sh
usage; update the line that currently runs sudo dnf install -y
microshift-io-dependencies-repo to sudo dnf install -y
microshift-io-dependencies in docs/run.md.
In `@src/copr/cni/build.sh`:
- Around line 50-59: The unquoted ${version} variable is used in several path
constructions (mkdir, tar -C, cp, tar czf, mv) in build.sh and triggers
shellcheck SC2086; update every occurrence to use quoted expansions like
"${version}" (e.g., containernetworking-plugins-"${version}"/x86_64) so paths
with spaces or glob chars are safe, and ensure all related commands (mkdir, tar
-C, cp, tar czf, mv) reference the quoted form consistently.
In `@src/copr/copr-cli.Containerfile`:
- Around line 3-7: The RUN installation line installs the wrong package name
"rpmbuild"; replace it with the correct Fedora package "rpm-build" in the RUN
command that installs "copr-cli jq rpmbuild" (so the token "rpmbuild" in that
RUN should be changed to "rpm-build") and keep the rest of the flags and cleanup
unchanged.
In `@src/copr/copr.mk`:
- Around line 120-126: The copr-cni make target is missing a .PHONY declaration,
uses the interactive -ti flags in the podman run, and the file lacks a final
newline; update the Makefile to add copr-cni to the .PHONY list (alongside other
phony targets), remove the -ti flags from the podman run invocation in the
copr-cni recipe so it runs non-interactively, and ensure the file ends with a
trailing newline character so POSIX tools behave correctly.
- Around line 112-118: Add a .PHONY declaration for the copr-dependencies
target, remove the interactive TTY flags (-ti) from the podman run invocation
(use --rm only, or gate -t behind a tty check if colored output is needed), and
document or provide a default for OKD_VERSION_TAG in the Makefile variables/help
section so the copr-dependencies target has a defined value; update references
to COPR_SECRET_NAME and COPR_CLI_IMAGE only as needed to match the existing
variable names.
In `@src/copr/microshift-io-dependencies.sh`:
- Around line 26-37: The if condition incorrectly uses command substitution
causing SC2091 (if $(...) executes stdout rather than the pipeline exit status);
change the condition to test the pipeline directly—invoke the pipeline starting
with copr-cli list-packages "${COPR_REPO_NAME}" | jq -r '.[].name' | grep -q
"${_package_name}" in the if statement (without $()), so the exit code controls
the branch; keep the inner logic that sets existing_package_version (from
copr-cli get-package ... | jq -r
'.latest_succeeded_build.source_package.version') and compares it to
"${pkg_version}-1" as before.
- Around line 88-91: The script currently calls copr-cli build with a local
.spec path (copr-cli build "${COPR_REPO_NAME}" "${dest}/${_package_name}.spec"),
but copr-cli expects an SRPM or URL; change the flow to create an SRPM from the
spec (e.g., run rpmbuild -ba /path/to/"${_package_name}.spec" or the project's
SRPM build command to produce an .src.rpm) and then call copr-cli build with
that SRPM path (copr-cli build "${COPR_REPO_NAME}"
"/path/to/${_package_name}-*.src.rpm"); keep the subsequent copr-cli
regenerate-repos call the same. Ensure variables (COPR_REPO_NAME, dest,
_package_name) are used to locate the produced SRPM.
🧹 Nitpick comments (3)
src/copr/copr-cli.Containerfile (1)
9-10:containernetworking-plugins.specis copied to both/and/cni/.Line 9 copies the spec to
/and line 10 copies it again to/cni/. Thebuild.shscript references it relative to its own directory ($_scriptdir), which is/cni/. The copy to/on line 9 appears redundant.Proposed fix
-COPY create-build.sh microshift-io-dependencies.sh cni/containernetworking-plugins.spec / +COPY create-build.sh microshift-io-dependencies.sh / COPY cni/build.sh cni/containernetworking-plugins.spec /cni/src/copr/microshift-io-dependencies.sh (1)
73-74:gpgcheck=0disables signature verification on the repo.The generated
.repofiles disable GPG checking. If this is intentional for beta mirrors, consider adding a comment in the spec explaining why.src/copr/cni/build.sh (1)
41-42: No cleanup trap fortemp_diron early exit.If the script fails between
mktempand the finalrm -rf, the temp directory persists. Not critical in CI containers, but a trap would be more robust.Suggested addition after line 41
temp_dir="$(mktemp -d "/tmp/containernetworking-plugins-${version}.XXXXXX")" +trap 'rm -rf "${temp_dir}"' EXIT cp "${_scriptdir}/containernetworking-plugins.spec" "${temp_dir}/"
Comment on lines
+112
to
+118
| copr-dependencies: copr-cfg-ensure-podman-secret copr-cli | ||
| @echo "Building RPM with MicroShift dependencies repositories configuration" | ||
| sudo podman run \ | ||
| --rm -ti \ | ||
| --secret ${COPR_SECRET_NAME},target=/root/.config/copr \ | ||
| "${COPR_CLI_IMAGE}" \ | ||
| /microshift-io-dependencies.sh "${OKD_VERSION_TAG}" "${COPR_REPO_NAME}" |
There was a problem hiding this comment.
Missing .PHONY and -ti will break in CI.
- Add
.PHONY: copr-dependencies— every other target in this file declares it. -tiallocates a TTY + interactive stdin. GitHub Actions runners have no TTY, so this will fail or emit warnings. The other targets in this file use only--rm. Drop-ti(or use-tonly if truly needed for colored output, gated on[ -t 0 ]).OKD_VERSION_TAGis undocumented in the help target and has no default — consider adding it to the variables section.
Proposed fix
+.PHONY: copr-dependencies
copr-dependencies: copr-cfg-ensure-podman-secret copr-cli
`@echo` "Building RPM with MicroShift dependencies repositories configuration"
sudo podman run \
- --rm -ti \
+ --rm \
--secret ${COPR_SECRET_NAME},target=/root/.config/copr \
"${COPR_CLI_IMAGE}" \
/microshift-io-dependencies.sh "${OKD_VERSION_TAG}" "${COPR_REPO_NAME}"🤖 Prompt for AI Agents
In `@src/copr/copr.mk` around lines 112 - 118, Add a .PHONY declaration for the
copr-dependencies target, remove the interactive TTY flags (-ti) from the podman
run invocation (use --rm only, or gate -t behind a tty check if colored output
is needed), and document or provide a default for OKD_VERSION_TAG in the
Makefile variables/help section so the copr-dependencies target has a defined
value; update references to COPR_SECRET_NAME and COPR_CLI_IMAGE only as needed
to match the existing variable names.
Comment on lines
+120
to
+126
| copr-cni: copr-cfg-ensure-podman-secret copr-cli | ||
| @echo "Building RPM with CNI plugins" | ||
| sudo podman run \ | ||
| --rm -ti \ | ||
| --secret ${COPR_SECRET_NAME},target=/root/.config/copr \ | ||
| "${COPR_CLI_IMAGE}" \ | ||
| /cni/build.sh "${COPR_REPO_NAME}" |
There was a problem hiding this comment.
Same issues: missing .PHONY and -ti; also no trailing newline.
Same fixes as copr-dependencies above. Additionally, the file lacks a final newline which can cause POSIX tools to misbehave.
Proposed fix
+.PHONY: copr-cni
copr-cni: copr-cfg-ensure-podman-secret copr-cli
`@echo` "Building RPM with CNI plugins"
sudo podman run \
- --rm -ti \
+ --rm \
--secret ${COPR_SECRET_NAME},target=/root/.config/copr \
"${COPR_CLI_IMAGE}" \
/cni/build.sh "${COPR_REPO_NAME}"
+🤖 Prompt for AI Agents
In `@src/copr/copr.mk` around lines 120 - 126, The copr-cni make target is missing
a .PHONY declaration, uses the interactive -ti flags in the podman run, and the
file lacks a final newline; update the Makefile to add copr-cni to the .PHONY
list (alongside other phony targets), remove the -ti flags from the podman run
invocation in the copr-cni recipe so it runs non-interactively, and ensure the
file ends with a trailing newline character so POSIX tools behave correctly.
pmtk
changed the title
pmtk
force-pushed
the
copr-deps-2
branch
3 times, most recently
from
146d1e0 to
0afea9f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/copr/copr.mk (1)
115-128:⚠️ Potential issue | 🟠 MajorDrop
-tifrom the new Podman runs.These targets are called from GitHub Actions, where stdin is non-interactive.
podman run --rm -ti ...commonly fails with “the input device is not a TTY”, so the new dependency/CNI jobs can die before either script starts.Possible fix
sudo podman run \ - --rm -ti \ + --rm \ --secret ${COPR_SECRET_NAME},target=/root/.config/copr \ "${COPR_CLI_IMAGE}" \ /microshift-io-dependencies.sh "${OKD_VERSION_TAG}" "${COPR_REPO_NAME}" @@ sudo podman run \ - --rm -ti \ + --rm \ --secret ${COPR_SECRET_NAME},target=/root/.config/copr \ "${COPR_CLI_IMAGE}" \ /cni/build.sh "${COPR_REPO_NAME}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/copr/copr.mk` around lines 115 - 128, The podman runs in the copr.mk targets use the interactive flags "-ti" which cause failures in non-interactive CI; remove the "-ti" flags from the podman run invocations used to launch "${COPR_CLI_IMAGE}" (the run that executes /microshift-io-dependencies.sh and the copr-cni run that executes /cni/build.sh) so the commands become non-interactive and will not error with "input device is not a TTY" in GitHub Actions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/nightly-copr.yaml:
- Around line 17-56: Add the new job build-dependencies-rpm as a dependency of
the image/test job so the image build cannot start until both RPM producers
finish; specifically, update the build-and-test-microshift job to include needs:
[build-rpms, build-dependencies-rpm] (or append build-dependencies-rpm to
whatever existing needs list) so that build-dependencies-rpm (which contains the
CNI step) and build-rpms complete before metadata is consumed.
In `@src/copr/microshift-io-dependencies.sh`:
- Around line 39-82: The current logic builds rhocp_versions as a range and
writes a repo file that enables every minor from
${_minor_version_start}..${minor}, which allows dnf to pick newer incompatible
mirrors; change rhocp_versions to only include the single target minor (e.g.,
construct rhocp_versions="${major}.${minor}" instead of the seq loop) and update
the for-loop that emits the repo (the loop over %{rhocp_versions} and the repo
block using variable v) to only iterate that single minor so the generated
openshift-mirror-beta.repo enables just the exact matching OCP minor for the
MicroShift build.
---
Duplicate comments:
In `@src/copr/copr.mk`:
- Around line 115-128: The podman runs in the copr.mk targets use the
interactive flags "-ti" which cause failures in non-interactive CI; remove the
"-ti" flags from the podman run invocations used to launch "${COPR_CLI_IMAGE}"
(the run that executes /microshift-io-dependencies.sh and the copr-cni run that
executes /cni/build.sh) so the commands become non-interactive and will not
error with "input device is not a TTY" in GitHub Actions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9cb861a8-b63f-416c-9c36-75947300d2b5
📒 Files selected for processing (9)
.github/workflows/nightly-copr.yamldocs/run.mdsrc/copr/cni/build.shsrc/copr/cni/containernetworking-plugins.specsrc/copr/copr-cli.Containerfilesrc/copr/copr.mksrc/copr/create-build.shsrc/copr/microshift-io-dependencies.shsrc/quickrpm.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/run.md
- src/copr/create-build.sh
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/copr/copr.mk (1)
112-128:⚠️ Potential issue | 🟡 MinorRemove
-tiflags—GitHub Actions runners have no TTY.The
-tiflags at lines 116 and 125 will fail or warn in CI. Also ensure file ends with a newline.Proposed fix
.PHONY: copr-dependencies copr-dependencies: copr-cfg-ensure-podman-secret copr-cli `@echo` "Building RPM with MicroShift dependencies repositories configuration" sudo podman run \ - --rm -ti \ + --rm \ --secret ${COPR_SECRET_NAME},target=/root/.config/copr \ "${COPR_CLI_IMAGE}" \ /microshift-io-dependencies.sh "${OKD_VERSION_TAG}" "${COPR_REPO_NAME}" .PHONY: copr-cni copr-cni: copr-cfg-ensure-podman-secret copr-cli `@echo` "Building RPM with CNI plugins" sudo podman run \ - --rm -ti \ + --rm \ --secret ${COPR_SECRET_NAME},target=/root/.config/copr \ "${COPR_CLI_IMAGE}" \ /cni/build.sh "${COPR_REPO_NAME}" +🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/copr/copr.mk` around lines 112 - 128, In the Makefile targets copr-dependencies and copr-cni remove the interactive/TTY flags "-ti" from the sudo podman run commands (the lines invoking "${COPR_CLI_IMAGE}" with /microshift-io-dependencies.sh and /cni/build.sh) so CI runners without a TTY won’t fail or warn, and add a trailing newline at the end of src/copr/copr.mk; keep the rest of the podman invocation and secret/arg variables (COPR_SECRET_NAME, COPR_CLI_IMAGE, COPR_REPO_NAME, OKD_VERSION_TAG) unchanged.
🧹 Nitpick comments (3)
src/copr/cni/build.sh (2)
41-84: Temp directory not cleaned on failure.If the script fails between lines 41-82, the temp directory persists. Add a trap for cleanup.
Proposed fix
temp_dir="$(mktemp -d "/tmp/containernetworking-plugins-${version}.XXXXXX")" +trap 'rm -rf "${temp_dir}"' EXIT cp "${_scriptdir}/containernetworking-plugins.spec" "${temp_dir}/"Then remove the explicit cleanup at the end:
popd >/dev/null -rm -rf "${temp_dir}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/copr/cni/build.sh` around lines 41 - 84, The temp directory created in temp_dir="$(mktemp -d ...)" is not cleaned if the script exits early; add a cleanup trap and function (e.g., cleanup() { popd >/dev/null || true; rm -rf "${temp_dir}"; }) right after temp_dir is created and register it with trap 'cleanup' EXIT ERR to ensure popd and rm -rf run on any exit; then remove the final explicit rm -rf/popd at the end (or keep popd only inside cleanup) so cleanup is centralized and always runs even on failure.
27-27: Consider usingjq --argfor variable interpolation.While safe here (hardcoded value), the idiomatic jq approach avoids shell quoting issues:
Proposed fix
-cni_pkg="$(copr-cli list-packages "${COPR_REPO_NAME}" | jq -r '.[] | select(.name == "'${_package_name}'")')" +cni_pkg="$(copr-cli list-packages "${COPR_REPO_NAME}" | jq -r --arg name "${_package_name}" '.[] | select(.name == $name)')"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/copr/cni/build.sh` at line 27, The command building cni_pkg uses shell interpolation into jq which can be fragile; update the call that constructs cni_pkg (the line invoking copr-cli list-packages "${COPR_REPO_NAME}" | jq -r ...) to pass the package name via jq's --arg (referencing _package_name and COPR_REPO_NAME) and use that $arg inside the jq filter instead of embedding the shell variable, ensuring safer/idiomatic variable interpolation.src/copr/copr-cli.Containerfile (1)
9-10: Redundant spec file copy to/.
cni/containernetworking-plugins.specis copied to both/(line 9) and/cni/(line 10). Thebuild.shscript only uses the/cni/copy via${_scriptdir}. Remove from line 9.Proposed fix
-COPY create-build.sh microshift-io-dependencies.sh cni/containernetworking-plugins.spec / +COPY create-build.sh microshift-io-dependencies.sh / COPY cni/build.sh cni/containernetworking-plugins.spec /cni/🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/copr/copr-cli.Containerfile` around lines 9 - 10, The Dockerfile copies cni/containernetworking-plugins.spec twice—once to / and again to /cni/—but the build uses the /cni/ location; remove the redundant copy from the first COPY (the line that currently copies create-build.sh microshift-io-dependencies.sh cni/containernetworking-plugins.spec /) so only create-build.sh and microshift-io-dependencies.sh are copied there and leave the second COPY that places cni/containernetworking-plugins.spec into /cni/ (used by cni/build.sh via ${_scriptdir}).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/copr/copr.mk`:
- Around line 112-128: In the Makefile targets copr-dependencies and copr-cni
remove the interactive/TTY flags "-ti" from the sudo podman run commands (the
lines invoking "${COPR_CLI_IMAGE}" with /microshift-io-dependencies.sh and
/cni/build.sh) so CI runners without a TTY won’t fail or warn, and add a
trailing newline at the end of src/copr/copr.mk; keep the rest of the podman
invocation and secret/arg variables (COPR_SECRET_NAME, COPR_CLI_IMAGE,
COPR_REPO_NAME, OKD_VERSION_TAG) unchanged.
---
Nitpick comments:
In `@src/copr/cni/build.sh`:
- Around line 41-84: The temp directory created in temp_dir="$(mktemp -d ...)"
is not cleaned if the script exits early; add a cleanup trap and function (e.g.,
cleanup() { popd >/dev/null || true; rm -rf "${temp_dir}"; }) right after
temp_dir is created and register it with trap 'cleanup' EXIT ERR to ensure popd
and rm -rf run on any exit; then remove the final explicit rm -rf/popd at the
end (or keep popd only inside cleanup) so cleanup is centralized and always runs
even on failure.
- Line 27: The command building cni_pkg uses shell interpolation into jq which
can be fragile; update the call that constructs cni_pkg (the line invoking
copr-cli list-packages "${COPR_REPO_NAME}" | jq -r ...) to pass the package name
via jq's --arg (referencing _package_name and COPR_REPO_NAME) and use that $arg
inside the jq filter instead of embedding the shell variable, ensuring
safer/idiomatic variable interpolation.
In `@src/copr/copr-cli.Containerfile`:
- Around line 9-10: The Dockerfile copies cni/containernetworking-plugins.spec
twice—once to / and again to /cni/—but the build uses the /cni/ location; remove
the redundant copy from the first COPY (the line that currently copies
create-build.sh microshift-io-dependencies.sh
cni/containernetworking-plugins.spec /) so only create-build.sh and
microshift-io-dependencies.sh are copied there and leave the second COPY that
places cni/containernetworking-plugins.spec into /cni/ (used by cni/build.sh via
${_scriptdir}).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f4b006fe-7cfd-4086-8518-a3ec324c2923
📒 Files selected for processing (9)
.github/workflows/nightly-copr.yamldocs/run.mdsrc/copr/cni/build.shsrc/copr/cni/containernetworking-plugins.specsrc/copr/copr-cli.Containerfilesrc/copr/copr.mksrc/copr/create-build.shsrc/copr/microshift-io-dependencies.shsrc/quickrpm.sh
🚧 Files skipped from review as they are similar to previous changes (3)
- src/quickrpm.sh
- src/copr/microshift-io-dependencies.sh
- docs/run.md
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/copr/cni/build.sh (1)
41-44: Use an EXIT trap for temp-dir cleanup.Cleanup currently runs only at the end of the success path. Add a trap so failures don’t leak temp directories.
Suggested refactor
temp_dir="$(mktemp -d "/tmp/containernetworking-plugins-${version}.XXXXXX")" cp "${_scriptdir}/containernetworking-plugins.spec" "${temp_dir}/" pushd "${temp_dir}" >/dev/null +cleanup() { + popd >/dev/null || true + rm -rf "${temp_dir}" +} +trap cleanup EXIT @@ -popd >/dev/null -rm -rf "${temp_dir}"Also applies to: 83-84
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/copr/cni/build.sh` around lines 41 - 44, The script currently creates temp_dir with temp_dir="$(mktemp -d ...)" and copies files then pushd into it, but cleanup only runs on the success path; add an EXIT trap to remove the temp_dir on script exit (and optionally on ERR) to avoid leaking directories: define a cleanup function that pops the directory if needed and removes "${temp_dir}", then register it with trap 'cleanup' EXIT (or trap 'cleanup' ERR EXIT) immediately after temp_dir is created so any early exit also triggers removal; update references around temp_dir, pushd/popd and the existing cleanup at lines referencing 83-84 to rely on this trap..github/workflows/nightly-copr.yaml (1)
28-56: Consider deduplicating COPR-config bootstrap logic.The same 2-line secret-file setup is repeated across jobs. A small composite action (or Make target wrapper) would reduce drift.
Also applies to: 78-108, 186-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/nightly-copr.yaml around lines 28 - 56, The COPR_CONFIG secret-file bootstrap (echo "${COPR_CONFIG}" > /tmp/copr-config and the surrounding env/COPR_CONFIG usage) is duplicated across multiple jobs (see the jobs that call make copr-dependencies and make copr-cni); extract that two-line setup into a reusable unit — either create a small composite GitHub Action (e.g., actions/bootstrap-copr-config) or add a Makefile wrapper target (e.g., make copr-bootstrap) that writes the secret to /tmp/copr-config and exposes COPR_CONFIG=/tmp/copr-config, then replace the repeated blocks in the jobs that invoke the make targets copr-dependencies and copr-cni (and the other occurrences you noted) to call the new action/target instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/nightly-copr.yaml:
- Around line 31-37: The workflow currently writes the COPR_CONFIG secret to
/tmp/copr-config via plain redirection (echo "${COPR_CONFIG}" >
/tmp/copr-config) in multiple run blocks, creating a world-readable file; change
each occurrence to create the file with restricted permissions using a command
that sets mode 600 (e.g., use install -m 600 or equivalent) so the file is only
readable by the runner user, keeping the same variable (COPR_CONFIG) and target
path (/tmp/copr-config) and update all five occurrences in the run sections that
contain the COPR_CONFIG write.
In `@src/copr/cni/build.sh`:
- Around line 18-25: The curl call that sets latest_tag should be made fail-fast
and validated: change the curl invocation used to populate latest_tag (the
command assigning latest_tag) to include error flags (-fsSL) and a retry option
(--retry with a small count) so network/API failures surface immediately, then
check that latest_tag is non-empty before using version (the variable derived
from latest_tag) and if it's empty print a clear error via echo to stderr and
exit non-zero; ensure the validation occurs right after the latest_tag
assignment and before computing version.
---
Nitpick comments:
In @.github/workflows/nightly-copr.yaml:
- Around line 28-56: The COPR_CONFIG secret-file bootstrap (echo
"${COPR_CONFIG}" > /tmp/copr-config and the surrounding env/COPR_CONFIG usage)
is duplicated across multiple jobs (see the jobs that call make
copr-dependencies and make copr-cni); extract that two-line setup into a
reusable unit — either create a small composite GitHub Action (e.g.,
actions/bootstrap-copr-config) or add a Makefile wrapper target (e.g., make
copr-bootstrap) that writes the secret to /tmp/copr-config and exposes
COPR_CONFIG=/tmp/copr-config, then replace the repeated blocks in the jobs that
invoke the make targets copr-dependencies and copr-cni (and the other
occurrences you noted) to call the new action/target instead.
In `@src/copr/cni/build.sh`:
- Around line 41-44: The script currently creates temp_dir with
temp_dir="$(mktemp -d ...)" and copies files then pushd into it, but cleanup
only runs on the success path; add an EXIT trap to remove the temp_dir on script
exit (and optionally on ERR) to avoid leaking directories: define a cleanup
function that pops the directory if needed and removes "${temp_dir}", then
register it with trap 'cleanup' EXIT (or trap 'cleanup' ERR EXIT) immediately
after temp_dir is created so any early exit also triggers removal; update
references around temp_dir, pushd/popd and the existing cleanup at lines
referencing 83-84 to rely on this trap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 39a5a86f-080c-4e4a-8e4f-39c59464e7f2
📒 Files selected for processing (9)
.github/workflows/nightly-copr.yamldocs/run.mdsrc/copr/cni/build.shsrc/copr/cni/containernetworking-plugins.specsrc/copr/copr-cli.Containerfilesrc/copr/copr.mksrc/copr/create-build.shsrc/copr/microshift-io-dependencies.shsrc/quickrpm.sh
🚧 Files skipped from review as they are similar to previous changes (5)
- src/copr/copr.mk
- docs/run.md
- src/copr/create-build.sh
- src/copr/microshift-io-dependencies.sh
- src/copr/copr-cli.Containerfile
Comment on lines
+18
to
+25
| latest_tag=$(curl -L \ | ||
| -H "Accept: application/vnd.github+json" \ | ||
| -H "X-GitHub-Api-Version: 2022-11-28" \ | ||
| https://api.github.com/repos/containernetworking/plugins/releases/latest | jq -r '.tag_name') | ||
|
|
||
| echo "### containernetworking/plugins latest tag: '${latest_tag}'" | ||
| version="${latest_tag#v}" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if file exists and read lines around 18-25
if [ -f src/copr/cni/build.sh ]; then
echo "=== File exists ==="
wc -l src/copr/cni/build.sh
echo "=== Lines 15-30 ==="
sed -n '15,30p' src/copr/cni/build.sh
else
echo "File not found"
find . -name "build.sh" -type f 2>/dev/null | head -20
fiRepository: microshift-io/microshift
Length of output: 907
🏁 Script executed:
# Check if shellcheck is available and test the current code snippet
shellcheck --version 2>/dev/null || echo "shellcheck not available in sandbox"Repository: microshift-io/microshift
Length of output: 206
🏁 Script executed:
# Create a test script with the current code snippet and check with shellcheck
cat > /tmp/current_code.sh << 'EOF'
#!/bin/bash
latest_tag=$(curl -L \
-H "Accept: application/vnd.github+json" \
-H "X-GitHub-Api-Version: 2022-11-28" \
https://api.github.com/repos/containernetworking/plugins/releases/latest | jq -r '.tag_name')
echo "### containernetworking/plugins latest tag: '${latest_tag}'"
version="${latest_tag#v}"
EOF
echo "=== Shellcheck on current code ==="
shellcheck /tmp/current_code.sh || echo "Exit code: $?"Repository: microshift-io/microshift
Length of output: 371
🏁 Script executed:
# Create a test script with the suggested fix and check with shellcheck
cat > /tmp/suggested_code.sh << 'EOF'
#!/bin/bash
latest_tag="$(curl -fsSL --retry 3 --retry-delay 2 \
-H "Accept: application/vnd.github+json" \
-H "X-GitHub-Api-Version: 2022-11-28" \
https://api.github.com/repos/containernetworking/plugins/releases/latest | jq -r '.tag_name // empty')"
if [[ -z "${latest_tag}" ]]; then
echo "ERROR: unable to resolve latest containernetworking/plugins tag"
exit 1
fi
echo "### containernetworking/plugins latest tag: '${latest_tag}'"
version="${latest_tag#v}"
EOF
echo "=== Shellcheck on suggested code ==="
shellcheck /tmp/suggested_code.sh || echo "Exit code: $?"Repository: microshift-io/microshift
Length of output: 376
🏁 Script executed:
# Verify the full file passes shellcheck
echo "=== Shellcheck on full file ==="
shellcheck src/copr/cni/build.sh || echo "Exit code: $?"Repository: microshift-io/microshift
Length of output: 99
Add fail-fast validation for GitHub API response.
The current code silently accepts empty or null tag_name from the API, causing failures later in copr-cli calls with unclear error messages. Add curl error flags (-fsSL, --retry), validate that latest_tag is non-empty, and exit immediately if resolution fails.
Suggested fix
-latest_tag=$(curl -L \
+latest_tag="$(curl -fsSL --retry 3 --retry-delay 2 \
-H "Accept: application/vnd.github+json" \
-H "X-GitHub-Api-Version: 2022-11-28" \
- https://api.github.com/repos/containernetworking/plugins/releases/latest | jq -r '.tag_name')
+ https://api.github.com/repos/containernetworking/plugins/releases/latest | jq -r '.tag_name // empty')"
+
+if [[ -z "${latest_tag}" ]]; then
+ echo "ERROR: unable to resolve latest containernetworking/plugins tag"
+ exit 1
+fi📝 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.
Suggested change
| latest_tag=$(curl -L \ | |
| -H "Accept: application/vnd.github+json" \ | |
| -H "X-GitHub-Api-Version: 2022-11-28" \ | |
| https://api.github.com/repos/containernetworking/plugins/releases/latest | jq -r '.tag_name') | |
| echo "### containernetworking/plugins latest tag: '${latest_tag}'" | |
| version="${latest_tag#v}" | |
| latest_tag="$(curl -fsSL --retry 3 --retry-delay 2 \ | |
| -H "Accept: application/vnd.github+json" \ | |
| -H "X-GitHub-Api-Version: 2022-11-28" \ | |
| https://api.github.com/repos/containernetworking/plugins/releases/latest | jq -r '.tag_name // empty')" | |
| if [[ -z "${latest_tag}" ]]; then | |
| echo "ERROR: unable to resolve latest containernetworking/plugins tag" | |
| exit 1 | |
| fi | |
| echo "### containernetworking/plugins latest tag: '${latest_tag}'" | |
| version="${latest_tag#v}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/copr/cni/build.sh` around lines 18 - 25, The curl call that sets
latest_tag should be made fail-fast and validated: change the curl invocation
used to populate latest_tag (the command assigning latest_tag) to include error
flags (-fsSL) and a retry option (--retry with a small count) so network/API
failures surface immediately, then check that latest_tag is non-empty before
using version (the variable derived from latest_tag) and if it's empty print a
clear error via echo to stderr and exit non-zero; ensure the validation occurs
right after the latest_tag assignment and before computing version.
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.
Addresses: #165
Summary by CodeRabbit
New Features
Documentation
Chores