Publish upstream rpms on COPR #151
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds COPR/SRPM-based RPM build and release flow and parameterizes the RPM builder image: new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant GHA as GitHub Actions
participant SRPM as SRPM Builder (packaging/srpm)
participant ART as Actions Artifacts
participant COPRCLI as COPR CLI (copr-cli container / create-build.sh)
participant COPR as COPR Service
participant RPM_ASM as rpms-copr container
participant REPO as RPM Repo Artifact
GHA->>SRPM: start SRPM build (mount outputs)
SRPM-->>ART: emit SRPMs + /srpms/build.txt (version)
GHA->>COPRCLI: run create-build (mount SRPMs)
COPRCLI->>COPR: copr-cli create-build --srpm ...
COPR-->>COPRCLI: returns build ID
COPRCLI-->>ART: persist build ID (/srpms/build.txt)
GHA->>COPRCLI: watch COPR build (poll)
COPRCLI->>COPR: query build status
COPR-->>COPRCLI: build completes
GHA->>RPM_ASM: run rpms-copr (COPR build id)
RPM_ASM->>COPR: download COPR RPMs
COPR-->>RPM_ASM: RPM artifacts
RPM_ASM->>REPO: place RPMs + run createrepo
RPM_ASM-->>ART: provide RPM repo artifact
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/quickrpm.sh (1)
8-85: Tighten COPR flow (dnf copr plugin, error handling, and unusedTAG)The overall COPR-based install flow looks good, but a few details are worth tightening:
dnf copr enable -y "${COPR}"implicitly requires thecoprplugin (dnf-plugins-core). On a minimal RHEL/Fedora/CentOS install this may not be present and will cause a somewhat opaque failure. Consider either installingdnf-plugins-core(ordnf-command(copr)) incheck_prerequisites()or emitting a clear error ifdnf copris unavailable.- If the COPR repo has no
microshiftpackage, thednf ... repoquery microshiftpipeline will fail underset -euo pipefailwith little context. Wrapping this in explicit error handling (and checking thatminor_versionis non-empty) would give a much clearer message like “no microshift package found in ${COPR}”.- After switching to COPR, the
TAGresolution block (GitHub releases +jq) is now unused and only adds an extra network dependency and a possible failure path. It can likely be removed, along with thednf install -y jq, unless you still plan to useTAGelsewhere.
🧹 Nitpick comments (9)
src/topolvm/topolvm.spec (1)
65-67: Consider ensuring directory ownership for%{_datadir}/microshift/releaseThe release-info JSONs are installed under
%{_datadir}/microshift/releaseand listed in%files topolvm-release-info. Just confirm that some package (either this one ormicroshift-release-info) also owns themicroshift/andmicroshift/release/directories to avoid rpm/repocheck complaints about orphaned directories.src/image/prebuild.sh (1)
5-6: Validate/limitARCHwhen allowing env overrideLetting callers override
ARCHis useful, but if they pass values other thanx86_64/aarch64,${UNAME_TO_GOARCH_MAP[${ARCH}]}becomes empty and file lookups likerelease-${ARCH}.jsonor10-microshift_${...}.confwill fail in non-obvious ways. Consider validatingARCHagainst the supported set (or documenting the expected values clearly) to fail fast with a better error message.packaging/microshift-runner.Containerfile (1)
4-6: Address DL3006 on untagged builder image (or explicitly ignore it)Using a parameterized builder image is reasonable, but hadolint now flags
FROM localhost/${RPM_BUILDER_IMAGE}for lacking an explicit tag. Since the tag is effectively dynamic, you likely need either:
- a documented convention that
${RPM_BUILDER_IMAGE}always includes a tag (e.g.rpm-local-builder:latest), or- a
# hadolint ignore=DL3006comment above thisFROMto keep the linter clean while retaining flexibility.src/rpm/create_repos.sh (1)
7-22: Good factoring of deps repo creation; optionally validaterepo_versionExtracting
create_deps_repo()and reusing it from both-createand-deps-onlyreduces duplication and cleanly supports the new Quick Start flow. You might optionally add a simple non-empty check forrepo_versionin the-deps-onlypath to fail fast with a clearer message if the caller forgets the argument.Also applies to: 37-38, 73-76
packaging/srpm.Containerfile (1)
49-50: Consider consolidating consecutive RUN instructions.Optional: Merge the two architecture-specific prebuild runs to reduce layers and address DL3059 warnings.
-RUN ARCH="x86_64" "${USHIFT_PREBUILD_SCRIPT}" --replace "${OKD_RELEASE_IMAGE_X86_64}" "${OKD_VERSION_TAG}" -RUN ARCH="aarch64" "${USHIFT_PREBUILD_SCRIPT}" --replace "${OKD_RELEASE_IMAGE_AARCH64}" "${OKD_VERSION_TAG}" +RUN ARCH="x86_64" "${USHIFT_PREBUILD_SCRIPT}" --replace "${OKD_RELEASE_IMAGE_X86_64}" "${OKD_VERSION_TAG}" && \ + ARCH="aarch64" "${USHIFT_PREBUILD_SCRIPT}" --replace "${OKD_RELEASE_IMAGE_AARCH64}" "${OKD_VERSION_TAG}"src/image/modify-spec.py (1)
38-43: Consider iterable unpacking (per static analysis).-install_keywords_to_remove = pkgs_to_remove + [ +install_keywords_to_remove = [ + *pkgs_to_remove, 'lib/tuned', '05-high-performance-runtime.conf', 'microshift-baseline', 'microshift-tuned', ].github/workflows/release.yaml (2)
186-189: Inconsistent artifact action versions.Line 89 uses
download-artifact@v5, but here uses@v4. Standardize to avoid compatibility issues.- - uses: actions/download-artifact@v4 + - uses: actions/download-artifact@v5
49-59: Consider cleanup of COPR config file.Sensitive config is written to
/tmp/copr-config. While runners are ephemeral, explicit cleanup after use is good hygiene.make copr-watch-build \ SRPM_WORKDIR=/mnt/srpm \ COPR_REPO_NAME="${{ inputs.copr-repo }}" + + rm -f /tmp/copr-configsrc/copr/copr.mk (1)
27-33: Consider using--replaceflag for cleaner atomic updates.Podman's
secret createcommand supports the--replaceflag, which atomically updates or creates a secret. This is preferable to the current remove-then-create pattern as it eliminates the conditional check and potential race conditions..PHONY: copr-cfg-ensure-podman-secret copr-cfg-ensure-podman-secret: @echo "Ensuring the COPR secret is available and is up to date" - if sudo podman secret exists "${COPR_SECRET_NAME}"; then \ - sudo podman secret rm "${COPR_SECRET_NAME}" ; \ - fi && \ - sudo podman secret create "${COPR_SECRET_NAME}" "${COPR_CONFIG}" + sudo podman secret create --replace "${COPR_SECRET_NAME}" "${COPR_CONFIG}"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.github/actions/build/action.yaml(3 hunks).github/actions/prebuild/action.yaml(1 hunks).github/workflows/release.md(1 hunks).github/workflows/release.yaml(3 hunks)Makefile(4 hunks)docs/run.md(2 hunks)docs/workflows.md(2 hunks)packaging/microshift-builder.Containerfile(1 hunks)packaging/microshift-runner.Containerfile(1 hunks)packaging/rpms-copr.Containerfile(1 hunks)packaging/srpm.Containerfile(1 hunks)src/copr/copr-cli.Containerfile(1 hunks)src/copr/copr.mk(1 hunks)src/copr/create-build.sh(1 hunks)src/image/build-rpms.sh(1 hunks)src/image/modify-spec.py(4 hunks)src/image/prebuild.sh(1 hunks)src/quickrpm.sh(3 hunks)src/rpm/create_repos.sh(3 hunks)src/topolvm/topolvm.spec(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📚 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.shsrc/quickrpm.shpackaging/rpms-copr.Containerfile.github/actions/build/action.yaml.github/workflows/release.mdpackaging/microshift-builder.Containerfilesrc/topolvm/topolvm.specMakefilesrc/copr/create-build.shpackaging/srpm.Containerfiledocs/run.md.github/workflows/release.yamldocs/workflows.mdsrc/image/modify-spec.py
📚 Learning: 2025-11-26T06:46:33.353Z
Learnt from: ggiguash
Repo: microshift-io/microshift PR: 137
File: src/quickrpm.sh:83-98
Timestamp: 2025-11-26T06:46:33.353Z
Learning: In the microshift-io/microshift repository, the quickrpm.sh script only supports RHEL 9 or above (and equivalent CentOS/Fedora versions), where util-linux includes the `--nooverlap` flag for losetup.
Applied to files:
src/image/build-rpms.shsrc/quickrpm.shpackaging/rpms-copr.Containerfile.github/workflows/release.mdpackaging/microshift-builder.Containerfilesrc/topolvm/topolvm.specsrc/image/prebuild.shMakefilesrc/copr/create-build.shpackaging/srpm.Containerfiledocs/run.mddocs/workflows.md
📚 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/microshift-builder.Containerfilesrc/image/prebuild.shpackaging/srpm.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:
src/copr/copr.mk.github/actions/build/action.yaml.github/workflows/release.mdpackaging/microshift-builder.ContainerfileMakefilepackaging/srpm.Containerfilepackaging/microshift-runner.Containerfile.github/workflows/release.yamldocs/workflows.md
🪛 actionlint (1.7.9)
.github/workflows/release.yaml
117-117: missing input "okd-version-tag" which is required by action "build-microshift" defined at "./.github/actions/build". all required inputs are "build", "okd-version-tag", "ushift-gitref"
(action)
117-117: missing input "ushift-gitref" which is required by action "build-microshift" defined at "./.github/actions/build". all required inputs are "build", "okd-version-tag", "ushift-gitref"
(action)
🪛 GitHub Actions: linters
packaging/rpms-copr.Containerfile
[warning] 11-11: DL3003 warning: Use WORKDIR to switch to a directory
[warning] 11-11: DL4006 warning: Set the SHELL option -o pipefail before RUN with a pipe in it. If you are using /bin/sh in an alpine image or if your shell is symlinked to busybox then consider explicitly setting your SHELL to /bin/ash, or disable this check
packaging/microshift-builder.Containerfile
[warning] 71-71: DL3059 info: Multiple consecutive RUN instructions. Consider consolidation.
packaging/srpm.Containerfile
[warning] 50-50: DL3059 info: Multiple consecutive RUN instructions. Consider consolidation.
[warning] 66-66: DL3059 info: Multiple consecutive RUN instructions. Consider consolidation.
[warning] 73-73: DL3059 info: Multiple consecutive RUN instructions. Consider consolidation.
[warning] 78-78: DL3059 info: Multiple consecutive RUN instructions. Consider consolidation.
packaging/microshift-runner.Containerfile
[warning] 6-6: DL3006 warning: Always tag the version of an image explicitly
🪛 Ruff (0.14.7)
src/image/modify-spec.py
38-43: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
⏰ 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). (9)
- GitHub Check: fedora-bootc (ubuntu-24.04)
- GitHub Check: isolated-network (ovnk, ubuntu-24.04, 1)
- GitHub Check: isolated-network (kindnet, ubuntu-24.04, 0)
- GitHub Check: fedora-bootc (ubuntu-24.04-arm)
- GitHub Check: ubuntu-rpm2deb
- GitHub Check: centos10-bootc
- GitHub Check: centos9-bootc
- GitHub Check: quick-start-and-clean (ubuntu-24.04)
- GitHub Check: quick-start-and-clean (ubuntu-24.04-arm)
🔇 Additional comments (15)
src/topolvm/topolvm.spec (1)
55-64: topolvm install block matches %files; paths and modes look consistentThe install commands correctly create the manifests directory, config drop-in, and greenboot check in the locations declared in
%files topolvm, with appropriate permissions. This should integrate cleanly with the main microshift package layout.src/image/build-rpms.sh (1)
74-82: Scope ofversion.txtto rpm/srpm targets looks correctWriting
MICROSHIFT_VERSIONseparately intoRPMS/version.txtandSRPMS/version.txtwithin their respective branches aligns the metadata with the artifacts actually built and keeps the behavior safe underset -euo pipefail.docs/workflows.md (1)
54-77: Documentation matches new SRPM/COPR-based release flowThe updated description of the MicroShift workflow (SRPM, COPR RPMs, DEBs, Bootc image) and the explicit COPR link/installation note are consistent and clear.
.github/actions/build/action.yaml (1)
31-38: Re-check prebuild gating andrpm-builderwiring for bootc-only or custom flowsGating the RPM build on
inputs.rpm-builder == 'rpm-local-builder'makes sense, but applying the same condition to theprebuildstep means that whenrpm-builderis set to a non-local value andinputs.buildstill includes"bootc-image"or"all", the bootc image build will run without the prebuild-installed tools and/mntlayout. That could easily break future “bootc-only via COPR” usages of this composite action.Also,
BUILDER_IMAGE=${{ inputs.rpm-builder }}now directly ties the make target’s builder image name to this input. Please double-check that all workflows calling this action pass a value that matches the actual locally-built builder image name expected by the Makefile/microshift-runner build.If the intent is “always run prebuild for any bootc-image build, but only run the local RPM build when rpm-builder == rpm-local-builder”, consider conditioning
prebuildoninputs.buildinstead ofrpm-builder.Also applies to: 46-52, 64-83
.github/actions/prebuild/action.yaml (1)
30-32: SRPM directory addition aligns with new build artifactsCreating
/mnt/srpmalongside the existing/mntdirectories is consistent with the new SRPM/COPR flow and keeps all large artifacts on the larger mount.src/copr/copr-cli.Containerfile (1)
1-3: LGTM!Simple and effective. Consider adding
--setopt=install_weak_deps=Falsefor consistency with other Containerfiles in this PR (e.g.,rpms-copr.Containerfile), but not required..github/workflows/release.md (1)
10-18: LGTM!Simplifying user commands by removing
OWNERis a good UX improvement. The COPR-based workflow now handles repository resolution internally.src/image/modify-spec.py (2)
94-99: LGTM!Good encapsulation of specfile creation with required macros. The docstring explaining why dummy macros are needed is helpful.
102-113: LGTM!Clean main block with clear flow: open → prune → merge → save.
packaging/microshift-builder.Containerfile (1)
52-76: LGTM - Unified spec modification flow.The reorganized build sequence correctly:
- Copies component specs to /tmp
- Merges them via modify-spec.py
- Disables checks before building
The hadolint DL3059 warning (line 71) is a low-priority lint. The
COPYon line 74 separates the RUNs, and consolidating would reduce readability. Consider suppressing with an inline comment if needed.docs/run.md (1)
39-57: Good coverage of COPR installation options.Clear examples for enabling COPR with architecture-specific chroots.
src/copr/copr.mk (1)
10-25: LGTM - Clean RPM extraction workflow.Proper mount/copy/umount sequence with fallback to temp directory.
Makefile (3)
25-40: Good refactoring for multi-arch support.Architecture-specific release images and overridable
BUILDER_IMAGEimprove flexibility. Inclusion ofcopr.mkcleanly separates COPR-related targets.
85-97: LGTM - SRPM build target.Correctly passes both architecture release images for multi-arch SRPM generation.
110-130: LGTM - Flexible builder image selection.The
RPM_BUILDER_IMAGEbuild arg enables switching between local and COPR-based builders. Error message correctly references both build paths.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packaging/srpm.Containerfile (2)
24-26: Duplicate ARG declaration.
USHIFT_BUILDRPMS_SCRIPTis declared at both line 24 and 26.ARG USHIFT_BUILDRPMS_SCRIPT=/tmp/build-rpms.sh ARG USHIFT_MODIFY_SPEC_SCRIPT=/tmp/modify-spec.py -ARG USHIFT_BUILDRPMS_SCRIPT=/tmp/build-rpms.sh
76-77: Missing/outputdirectory creation.The copy to
/output/will fail since the directory doesn't exist.-RUN "${USHIFT_BUILDRPMS_SCRIPT}" srpm && \ - cp ./_output/rpmbuild/SRPMS/* /output/ +RUN "${USHIFT_BUILDRPMS_SCRIPT}" srpm && \ + mkdir -p /output && \ + cp ./_output/rpmbuild/SRPMS/* /output/
🧹 Nitpick comments (1)
packaging/srpm.Containerfile (1)
35-41: Add-fflag to curl.Without
--fail, curl returns exit 0 on HTTP errors (404, 500), causing a confusing tar failure later.- curl -L -o /tmp/okd-client.tar.gz "${OKD_CLIENT_URL}" && \ + curl -fL -o /tmp/okd-client.tar.gz "${OKD_CLIENT_URL}" && \
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packaging/microshift-builder.Containerfile(1 hunks)packaging/microshift-runner.Containerfile(1 hunks)packaging/rpms-copr.Containerfile(1 hunks)packaging/srpm.Containerfile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packaging/rpms-copr.Containerfile
🧰 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/microshift-runner.Containerfilepackaging/microshift-builder.Containerfilepackaging/srpm.Containerfile
📚 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:
packaging/microshift-builder.Containerfilepackaging/srpm.Containerfile
📚 Learning: 2025-11-26T06:46:33.353Z
Learnt from: ggiguash
Repo: microshift-io/microshift PR: 137
File: src/quickrpm.sh:83-98
Timestamp: 2025-11-26T06:46:33.353Z
Learning: In the microshift-io/microshift repository, the quickrpm.sh script only supports RHEL 9 or above (and equivalent CentOS/Fedora versions), where util-linux includes the `--nooverlap` flag for losetup.
Applied to files:
packaging/microshift-builder.Containerfilepackaging/srpm.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/microshift-builder.Containerfile
⏰ 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). (9)
- GitHub Check: quick-start-and-clean (ubuntu-24.04-arm)
- GitHub Check: ubuntu-rpm2deb
- GitHub Check: isolated-network (kindnet, ubuntu-24.04, 0)
- GitHub Check: isolated-network (ovnk, ubuntu-24.04, 1)
- GitHub Check: quick-start-and-clean (ubuntu-24.04)
- GitHub Check: fedora-bootc (ubuntu-24.04)
- GitHub Check: fedora-bootc (ubuntu-24.04-arm)
- GitHub Check: centos10-bootc
- GitHub Check: centos9-bootc
🔇 Additional comments (5)
packaging/microshift-runner.Containerfile (1)
4-6: LGTM!Parameterizing the builder image via
RPM_BUILDER_IMAGEprovides flexibility. Thelocalhost/prefix correctly references locally-built images per Podman conventions.packaging/srpm.Containerfile (1)
49-50: Dual-arch prebuild in single image is appropriate for SRPMs.Running prebuild for both architectures ensures the SRPM contains image references for all target platforms.
packaging/microshift-builder.Containerfile (3)
52-56: Kindnet spec handling updated to temp location.Moving kindnet.spec to
/tmpfor processing by modify-spec.py is consistent with the new consolidated packaging flow.
58-63: Topolvm packaging mirrors kindnet pattern.Spec to
/tmp, assets/dropins/greenboot/release to appropriate packaging paths. Consistent structure.
64-75: Consolidated spec modification and build flow.The modify-spec.py merges component specs, sed disables RPM/SRPM checks, and build-rpms.sh handles all packages. The
hadolint ignore=DL3059comment appropriately silences the consecutive RUN warning for the separated build step.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packaging/srpm.Containerfile (1)
75-76: Missing/outputdirectory - build will fail.Line 76 copies to
/output/but this directory is never created.-RUN "${USHIFT_BUILDRPMS_SCRIPT}" srpm && \ - cp ./_output/rpmbuild/SRPMS/* /output/ +RUN mkdir -p /output && \ + "${USHIFT_BUILDRPMS_SCRIPT}" srpm && \ + cp ./_output/rpmbuild/SRPMS/* /output/
🧹 Nitpick comments (3)
packaging/srpm.Containerfile (1)
34-40: Consider adding checksum verification for downloaded binary.The OKD client tarball is downloaded and extracted without integrity verification. If the download URL changes or is compromised, malicious binaries could be installed.
.github/workflows/release.yaml (1)
66-72: Version mismatch between upload-artifact (v4) and download-artifact (v5).Using different major versions of the artifact actions may cause compatibility issues.
- - uses: actions/download-artifact@v5 + - uses: actions/download-artifact@v4Also applies to: 89-92
src/copr/copr.mk (1)
19-25: Image mount may leak on copy failure.If
cpfails, the image remains mounted. Consider adding cleanup or using a trap.outdir="$${RPM_OUTDIR:-$$(mktemp -d /tmp/microshift-rpms-XXXXXX)}" && \ mntdir="$$(sudo podman image mount "${COPR_BUILDER_IMAGE}")" && \ + trap "sudo podman image umount '${COPR_BUILDER_IMAGE}' 2>/dev/null || true" EXIT && \ sudo cp -r "$${mntdir}/home/microshift/microshift/_output/rpmbuild/RPMS/." "$${outdir}" && \ sudo podman image umount "${COPR_BUILDER_IMAGE}" && \ + trap - EXIT && \
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/release.yaml(3 hunks)docs/run.md(2 hunks)packaging/srpm.Containerfile(1 hunks)src/copr/copr.mk(1 hunks)src/copr/create-build.sh(1 hunks)
🧰 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:
docs/run.mdsrc/copr/create-build.shpackaging/srpm.Containerfile
📚 Learning: 2025-11-26T06:46:33.353Z
Learnt from: ggiguash
Repo: microshift-io/microshift PR: 137
File: src/quickrpm.sh:83-98
Timestamp: 2025-11-26T06:46:33.353Z
Learning: In the microshift-io/microshift repository, the quickrpm.sh script only supports RHEL 9 or above (and equivalent CentOS/Fedora versions), where util-linux includes the `--nooverlap` flag for losetup.
Applied to files:
docs/run.mdsrc/copr/create-build.shpackaging/srpm.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:
src/copr/copr.mk.github/workflows/release.yamlpackaging/srpm.Containerfile
🪛 LanguageTool
docs/run.md
[grammar] ~22-~22: Use a hyphen to join words.
Context: ...://okd.io/docs/operators/) | ## Package based systems (non-bootc) ### Installin...
(QB_NEW_EN_HYPHEN)
⏰ 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). (9)
- GitHub Check: quick-start-and-clean (ubuntu-24.04)
- GitHub Check: quick-start-and-clean (ubuntu-24.04-arm)
- GitHub Check: isolated-network (ovnk, ubuntu-24.04, 1)
- GitHub Check: fedora-bootc (ubuntu-24.04-arm)
- GitHub Check: centos9-bootc
- GitHub Check: ubuntu-rpm2deb
- GitHub Check: isolated-network (kindnet, ubuntu-24.04, 0)
- GitHub Check: fedora-bootc (ubuntu-24.04)
- GitHub Check: centos10-bootc
🔇 Additional comments (5)
src/copr/create-build.sh (1)
1-11: LGTM - validation added as previously suggested.The script now properly validates the build ID extraction before writing to
build.txt. Error handling withset -euo pipefailis in place.docs/run.md (1)
24-68: Documentation updates look good.The installation sections for Local RPMs, COPR, and DEB are well-structured with clear examples. Path references have been corrected.
.github/workflows/release.yaml (2)
46-64: COPR workflow integration looks solid.The COPR build flow correctly chains
copr-create-buildandcopr-watch-build. Consider adding explicit cleanup of/tmp/copr-configin a post step if security hardening is desired, though ephemeral runners mitigate the risk.
119-122: Verify action accepts "not-required" placeholder values.Passing literal string "not-required" for
ushift-gitrefandokd-version-tagis unconventional. Ensure.github/actions/build/action.yamlhandles these values correctly whenrpm-builder: rpm-copr-builderbypasses the typical build flow.src/copr/copr.mk (1)
72-79:copr-watch-buildcorrectly omits secret dependency.Unlike
copr-create-buildandcopr-delete-builds, watching a public build doesn't require authentication. The target appropriately skips the secret setup.
pmtk
force-pushed
the
coprx
branch
2 times, most recently
from
8ad8c74 to
19ceaaf
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/copr/copr.mk (1)
72-79: Missing dependency on secret setup.
copr-watch-builduses--secret ${COPR_SECRET_NAME}but doesn't depend oncopr-cfg-ensure-podman-secret, unlike other targets (lines 43, 52, 61)..PHONY: copr-watch-build -copr-watch-build: copr-cli +copr-watch-build: copr-cfg-ensure-podman-secret copr-cli @echo "Watching the COPR build"
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
.github/actions/build/action.yaml(3 hunks).github/actions/prebuild/action.yaml(1 hunks).github/workflows/release.md(1 hunks).github/workflows/release.yaml(3 hunks)docs/run.md(2 hunks)docs/workflows.md(2 hunks)packaging/microshift-builder.Containerfile(1 hunks)packaging/microshift-runner.Containerfile(1 hunks)packaging/rpms-copr.Containerfile(1 hunks)packaging/srpm.Containerfile(1 hunks)src/copr/copr.mk(1 hunks)src/copr/create-build.sh(1 hunks)src/quickrpm.sh(3 hunks)src/rpm/create_repos.sh(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- .github/actions/prebuild/action.yaml
- packaging/rpms-copr.Containerfile
- src/copr/create-build.sh
- .github/workflows/release.md
- packaging/srpm.Containerfile
🧰 Additional context used
🧠 Learnings (4)
📚 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:
packaging/microshift-builder.Containerfilesrc/quickrpm.sh.github/actions/build/action.yaml.github/workflows/release.yamldocs/run.mdsrc/rpm/create_repos.shdocs/workflows.md
📚 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/microshift-builder.Containerfilepackaging/microshift-runner.Containerfile.github/actions/build/action.yaml.github/workflows/release.yamlsrc/copr/copr.mkdocs/workflows.md
📚 Learning: 2025-11-26T06:46:33.353Z
Learnt from: ggiguash
Repo: microshift-io/microshift PR: 137
File: src/quickrpm.sh:83-98
Timestamp: 2025-11-26T06:46:33.353Z
Learning: In the microshift-io/microshift repository, the quickrpm.sh script only supports RHEL 9 or above (and equivalent CentOS/Fedora versions), where util-linux includes the `--nooverlap` flag for losetup.
Applied to files:
packaging/microshift-builder.Containerfilesrc/quickrpm.sh.github/actions/build/action.yamldocs/run.mddocs/workflows.md
📚 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/microshift-builder.Containerfile
⏰ 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). (9)
- GitHub Check: ubuntu-rpm2deb
- GitHub Check: isolated-network (ovnk, ubuntu-24.04, 1)
- GitHub Check: isolated-network (kindnet, ubuntu-24.04, 0)
- GitHub Check: quick-start-and-clean (ubuntu-24.04-arm)
- GitHub Check: fedora-bootc (ubuntu-24.04-arm)
- GitHub Check: fedora-bootc (ubuntu-24.04)
- GitHub Check: quick-start-and-clean (ubuntu-24.04)
- GitHub Check: centos10-bootc
- GitHub Check: centos9-bootc
🔇 Additional comments (27)
packaging/microshift-builder.Containerfile (3)
52-56: LGTM - Per-component spec handling for Kindnet.The Kindnet spec and assets are properly staged for the merge step.
58-63: LGTM - Per-component spec handling for TopoLVM.The TopoLVM spec and assets are properly staged for the merge step.
64-75: Verify the spec modification and build scripts.The modify-spec.py script merges component specs and disables RPM/SRPM checks. Ensure:
- The modify-spec.py script exists and correctly merges specs from the specified inputs
- The build-rpms.sh script accepts the "all" parameter as intended
- Disabling CHECK_RPMS/CHECK_SRPMS is intentional and won't mask packaging issues
src/copr/copr.mk (6)
1-8: LGTM - Variable definitions are reasonable.Note: COPR_BUILD_ID uses command substitution that will fail if the file doesn't exist, but this is acceptable since it's used as a default value.
10-25: LGTM - RPM extraction logic is correct.The target properly builds the image, mounts it, extracts RPMs, and cleans up.
27-33: LGTM - Secret management pattern is correct.The target ensures the secret is refreshed by removing any existing secret before creating a new one.
35-40: LGTM - COPR CLI container build.Simple and correct image build.
42-58: LGTM - COPR delete and regenerate targets.Both targets properly depend on secret setup and CLI availability.
60-70: LGTM - COPR create build target.Proper dependencies and volume mounts for SRPM and script.
packaging/microshift-runner.Containerfile (1)
4-6: LGTM - Builder image parameterization.The ARG properly parameterizes the builder image source, enabling flexible COPR-based builds.
src/quickrpm.sh (3)
8-8: LGTM - COPR variable introduction.The default value aligns with the COPR repository structure.
61-71: LGTM - COPR enablement and script downloads.The COPR repository is properly enabled and installation scripts are fetched from the source branch.
72-85: Verify COPR repository name transformation and version detection.The transformation from COPR name to repo identifier uses sed patterns that may need validation. Confirm:
- The transformation correctly handles both "@org/project" and "user/project" formats (the sed patterns
s,/,:,gands,@,group_,gappear correct for the documented cases, but edge cases should be tested)- Error handling exists if the COPR repo is empty or the microshift package is not available
- The
dnf repoquerycommand has appropriate error checking before passing the version tocreate_repos.shAdditionally, verify that
create_repos.shsupports the-deps-onlyflag as used on line 84.src/rpm/create_repos.sh (3)
8-8: LGTM - Refactored dependency repository creation.The new
create_deps_repofunction properly extracts the OpenShift mirror repo creation logic for reuse.Also applies to: 12-22
37-37: LGTM - Function call replaces inline code.Cleaner delegation to
create_deps_repo.
73-76: LGTM - New -deps-only option.Enables independent dependency repository creation, used by the COPR-based quickrpm.sh flow.
docs/workflows.md (1)
54-76: LGTM - Documentation updated for COPR workflow.The workflow description accurately reflects the new SRPM and COPR-based RPM build process.
.github/actions/build/action.yaml (4)
22-22: LGTM - String default values for consistency.Ensures consistent type handling across the workflow.
Also applies to: 26-26, 30-30
34-37: LGTM - RPM builder parameterization.The new input enables flexible selection between local and COPR-based builders.
47-47: LGTM - Conditional RPM build gating.Properly skips local RPM building when using the COPR-based builder.
Also applies to: 51-51
82-82: LGTM - Builder image propagation.Properly passes the builder selection to the image build process.
.github/workflows/release.yaml (5)
18-21: LGTM - COPR repository parameterization.The new input allows targeting different COPR repositories for testing or releases.
24-73: LGTM - COPR-based RPM build orchestration.The job properly builds SRPMs, creates COPR builds, waits for completion, and persists artifacts. Ensure the
COPR_CONFIGsecret is configured in the repository.
106-122: LGTM - COPR-based image build and test.The job properly fetches RPMs from COPR and builds the bootc image. The "not-required" values for
ushift-gitrefandokd-version-tagare acceptable since the COPR builder doesn't rebuild from source.
142-147: LGTM - DEB package conversion.Properly configured to convert existing RPMs to DEB without rebuilding.
224-233: LGTM - COPR repository regeneration.Properly refreshes the COPR repository metadata after the release, ensuring users can discover new packages.
docs/run.md (1)
32-37: Verify script parameters forcreate_repos.sh.The commands reference
create_repos.shwith-createand-deleteflags. Confirm these flags and parameter order match the script's implementation.
pmtk
force-pushed
the
coprx
branch
2 times, most recently
from
1c687e2 to
ecdcbb6
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/image/modify-spec.py (1)
78-86:len(sections) - 3insertion index is still fragile.Inserting extra sections at
len(sections) - 3assumes a fixed spec layout (e.g., last 3 sections are stable, with changelog at the end). If the base spec structure changes, new sections could end up in the wrong place without obvious errors. Prefer anchoring on a known section ID (e.g., the changelog or a specific marker) and inserting relative to that; fall back to appending if the anchor isn’t found.This concern was raised previously and still applies to the refactored helper.
packaging/srpm.Containerfile (1)
74-76: Create/outputbefore copying SRPMs (still missing).
cp ./_output/rpmbuild/SRPMS/* /output/will fail because/outputisn’t created anywhere in this Containerfile. You can fix this inline:RUN "${USHIFT_BUILDRPMS_SCRIPT}" srpm && \ mkdir -p /output && \ cp ./_output/rpmbuild/SRPMS/* /output/This also keeps it as a single layer, avoiding the earlier DL3059 warning.
docs/run.md (2)
39-52: COPR namespace should match the actual MicroShift COPR project.As of December 4, 2025, the public MicroShift COPR projects use the
@redhat-et/microshiftnamespace (and related variants likemicroshift-nightly), with EPEL 9 and Fedora 42+ chroots; I don’t see a COPR project named@microshift-io/microshift.(copr.fedorainfracloud.org)Unless there is a new COPR under
@microshift-io/microshiftthat will exist by release time, the examples here should point to@redhat-et/microshiftsodnf copr enableworks out of the box.Example fix:
-sudo dnf copr enable @microshift-io/microshift +sudo dnf copr enable @redhat-et/microshift @@ -sudo dnf copr enable @microshift-io/microshift epel-9-x86_64 -sudo dnf copr enable @microshift-io/microshift epel-9-aarch64 -sudo dnf copr enable @microshift-io/microshift fedora-42-x86_64 -sudo dnf copr enable @microshift-io/microshift fedora-42-aarch64 +sudo dnf copr enable @redhat-et/microshift epel-9-x86_64 +sudo dnf copr enable @redhat-et/microshift epel-9-aarch64 +sudo dnf copr enable @redhat-et/microshift fedora-42-x86_64 +sudo dnf copr enable @redhat-et/microshift fedora-42-aarch64Check whether a COPR project named "@microshift-io/microshift" exists today, and if not, confirm that "@redhat-et/microshift" is still the correct COPR for MicroShift users to enable.
59-68: Pass the parent RPM directory toinstall.shinstead of thedeb/subdirectory.Per the existing Debian workflow,
convert.shwrites.debartifacts into adeb/subdirectory under the RPM directory, andsrc/deb/install.shexpects the parent RPM directory, recursively finding.debfiles beneath it. Passing/tmp/microshift-rpms/debnarrows the search root and diverges from the intended contract. Based on learnings, this should be the parent directory, not thedeb/subfolder.Recommended change:
-DEB_REPO_DIR=/tmp/microshift-rpms/deb +DEB_REPO_DIR=/tmp/microshift-rpms sudo ./src/deb/install.sh "${DEB_REPO_DIR}"#!/bin/bash # Inspect install.sh to confirm it expects the parent directory and uses recursive find. fd -a 'install.sh' src/deb sed -n '1,160p' src/deb/install.sh 2>/dev/null || true
🧹 Nitpick comments (5)
.github/workflows/release.yaml (1)
46-64: Consider more robust COPR_CONFIG file emission.The
echo "${COPR_CONFIG}" > /tmp/copr-configpattern works but will interpolate any${VAR}-like content inside the secret. If the COPR config ever contains such sequences, prefer a here‑doc to avoid accidental expansion:cat > /tmp/copr-config <<'EOF' ${COPR_CONFIG} EOFMinor robustness tweak, not a blocker.
src/copr/copr.mk (1)
42-59: Guard against emptyCOPR_BUILDSwhen deleting builds.
copr-delete-buildsassumesCOPR_BUILDSis non‑empty. If it’s unset/empty,copr-cli delete-buildwill run with no IDs. Consider a simple guard:copr-delete-builds: copr-cfg-ensure-podman-secret copr-cli @echo "Deleting the COPR builds" @if [ -z "$${COPR_BUILDS:-}" ]; then \ echo "ERROR: COPR_BUILDS is empty; set it to one or more build IDs"; \ exit 1; \ fi; \ sudo podman run \ --rm \ --secret ${COPR_SECRET_NAME} \ "${COPR_CLI_IMAGE}" \ bash -c "copr-cli --config /run/secrets/copr-cfg delete-build ${COPR_BUILDS}"src/quickrpm.sh (2)
61-63: Ensurednf coprplugin is available or fail with a clearer message.
dnf copr enable -y "${COPR}"assumes the COPR plugin is installed. On some RHEL/Fedora derivatives it might not be. Consider either installingdnf-plugins-coreup front or checking fordnf coprand emitting a tailored error if missing.
118-126:TAGresolution appears unused now.The logic that resolves
TAG="latest"to the latest GitHub release no longer feeds into RPM fetching or any later step. If that’s intentional, consider removing this block (or reusing TAG somewhere visible) to avoid confusion.packaging/microshift-builder.Containerfile (1)
64-75: modify-spec + build-rpms sequencing is reasonable for this dedicated builder image.Merging the temporary Kindnet/TopoLVM specs into
microshift.spec, then disablingCHECK_RPMS/CHECK_SRPMSbefore invokingbuild-rpms.sh allgives a clear, linear build pipeline. Just be aware that thesedchange affects all subsequent uses ofmake-rpm.shinside this image, which is fine as long as this container is only used for the “no‑checks” path.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.github/actions/build/action.yaml(3 hunks).github/actions/prebuild/action.yaml(1 hunks).github/workflows/release.md(1 hunks).github/workflows/release.yaml(3 hunks)Makefile(4 hunks)docs/run.md(2 hunks)docs/workflows.md(2 hunks)packaging/microshift-builder.Containerfile(1 hunks)packaging/microshift-runner.Containerfile(1 hunks)packaging/rpms-copr.Containerfile(1 hunks)packaging/srpm.Containerfile(1 hunks)src/copr/copr-cli.Containerfile(1 hunks)src/copr/copr.mk(1 hunks)src/copr/create-build.sh(1 hunks)src/image/build-rpms.sh(1 hunks)src/image/modify-spec.py(4 hunks)src/image/prebuild.sh(1 hunks)src/quickrpm.sh(3 hunks)src/rpm/create_repos.sh(3 hunks)src/topolvm/topolvm.spec(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- src/image/prebuild.sh
- src/topolvm/topolvm.spec
- src/image/build-rpms.sh
- src/copr/copr-cli.Containerfile
- .github/workflows/release.md
- packaging/rpms-copr.Containerfile
- packaging/microshift-runner.Containerfile
- .github/actions/build/action.yaml
- Makefile
🧰 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/srpm.Containerfiledocs/workflows.mdpackaging/microshift-builder.Containerfile.github/workflows/release.yamlsrc/copr/copr.mk
📚 Learning: 2025-11-26T06:46:33.353Z
Learnt from: ggiguash
Repo: microshift-io/microshift PR: 137
File: src/quickrpm.sh:83-98
Timestamp: 2025-11-26T06:46:33.353Z
Learning: In the microshift-io/microshift repository, the quickrpm.sh script only supports RHEL 9 or above (and equivalent CentOS/Fedora versions), where util-linux includes the `--nooverlap` flag for losetup.
Applied to files:
packaging/srpm.Containerfilesrc/copr/create-build.shsrc/quickrpm.shdocs/workflows.mddocs/run.mdpackaging/microshift-builder.Containerfile
📚 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:
packaging/srpm.Containerfilesrc/copr/create-build.shsrc/quickrpm.shdocs/workflows.mddocs/run.mdsrc/image/modify-spec.pypackaging/microshift-builder.Containerfile.github/workflows/release.yaml
📚 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:
docs/run.mdpackaging/microshift-builder.Containerfile
🪛 Ruff (0.14.7)
src/image/modify-spec.py
38-43: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
⏰ 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). (4)
- GitHub Check: fedora-bootc (ubuntu-24.04)
- GitHub Check: fedora-bootc (ubuntu-24.04-arm)
- GitHub Check: quick-start-and-clean (ubuntu-24.04-arm)
- GitHub Check: quick-start-and-clean (ubuntu-24.04)
🔇 Additional comments (21)
.github/actions/prebuild/action.yaml (1)
31-31: LGTM! The addition of/mnt/srpmis a straightforward, safe change that aligns with SRPM-based workflows and follows the existing directory creation pattern..github/workflows/release.yaml (5)
18-21: COPR input wiring looks correct and consistent.
copr-repodefault matches the quickrpm default (@microshift-io/microshift) and is threaded into both COPR build and repo-regeneration steps, so the workflow stays self-consistent.
24-45: SRPM + OKD tag handling flow looks sound.Using
okd-version-tag != 'latest' && ... || steps.detect-okd-version.outputs.okd-version-tagcleanly handles the “latest vs explicit” tag choice, and persisting SRPM artifacts under/mnt/srpmgives a single source of truth for downstream jobs.
181-199: Version propagation viasrpm-artifactslooks good.Reusing
/tmp/srpm/version.txtinrelease-microshiftkeeps the manifest tag and release notes aligned with the SRPM/COPR build version.
224-233: COPR repo regeneration step is well placed.Regenerating the COPR repo after publishing the multi‑arch manifest ensures the COPR metadata is refreshed at the end of the release pipeline, and reusing the same
copr-repoinput keeps it in sync with the build step.
75-161: Fix Release step naming and artifact uploads to match.The step "Release RPM and DEB packages" only uploads DEB archives (
files: /mnt/release/microshift-debs-*.tgz). Either include the RPM tarball in thefileslist or rename/update the comment to reflect that only DEB packages are being released.Additionally, verify that the "not-required" placeholder values passed to
.github/actions/buildand.github/actions/build-debforushift-gitrefandokd-version-tagare handled safely within those composite actions. If those inputs are used unconditionally in the action implementations, pass actual values instead of placeholders for clarity and to prevent runtime surprises.docs/workflows.md (1)
54-56: Docs now accurately describe COPR-based RPM flow.The MicroShift workflow description and new COPR note correctly reflect that RPMs are published via COPR while DEBs and Bootc images are attached/published elsewhere. This matches the updated release pipeline.
Also applies to: 74-76
src/copr/copr.mk (3)
1-26: rpm-copr target and defaults look reasonable.Defaulting
COPR_CONFIGandCOPR_REPO_NAMEand derivingCOPR_BUILD_IDfrom${SRPM_WORKDIR}/build.txtline up with the workflow usage. The RPM extraction path under_output/rpmbuild/RPMSmatches the usual rpmbuild layout.
60-71: COPR build creation flow is cleanly containerized.Mounting
${SRPM_WORKDIR}read‑write, passingCOPR_REPO_NAMEas env, and drivingcreate-build.shvia the copr-cli image is a straightforward and debuggable integration point.
72-79: Watch-build target no longer needs secrets and looks fine.
copr-watch-buildjust consumes/srpms/build.txtand uses copr-cli without secrets, so the lighter dependency on onlycopr-cliis appropriate.src/copr/create-build.sh (1)
1-11: Build ID extraction and validation are solid.Strict mode plus the explicit
-z "${build}"check avoids silently writing an emptybuild.txtif copr-cli output changes or the build fails, which is exactly what the downstream Make targets need.src/quickrpm.sh (2)
8-8: COPR default aligns with workflow/docs.Using
COPR=${COPR:-"@${OWNER}/${REPO}"}keeps quickrpm’s default repo consistent with the release workflow and docs pointing at@microshift-io/microshift.
84-88: New-deps-onlypath integrates cleanly with create_repos.sh.Calling
create_repos.sh -deps-only "${minor_version}"nicely decouples OpenShift dependency repo setup from the MicroShift RPM source (now COPR), keeping quickrpm’s behavior aligned with the new packaging flow.src/rpm/create_repos.sh (1)
7-22: Nice separation of dependency repo creation.Extracting the OpenShift deps repo block into
create_deps_repo()simplifiescreate_repos()and makes the new-deps-onlymode straightforward to reuse.src/image/modify-spec.py (3)
14-22: Package removal and install filtering updates look good.Adding
sriovtopkgs_to_removeand derivinginstall_keywords_to_removefrom that list keeps the downstream pruning logic centralized and maintainable.Also applies to: 38-43
46-76: Helper for removing unsupported packages is clear and side-effect scoped.
remove_downstream_unsupported_packages()neatly encapsulates both section pruning and install section filtering, making the main flow easier to follow and simplifying future updates to the removal rules.
94-113: Main entrypoint and specfile handling are well-structured.Opening the downstream spec once, applying removals, then iteratively merging extra specfiles via
merge_specfile()keeps the script’s behavior explicit and testable. The dummy macro values inopen_specfile()are a pragmatic way to make parsing work outside the full RPM build context.packaging/microshift-builder.Containerfile (1)
52-63: Kindnet/TopoLVM spec and asset wiring looks consistent with the new flow.The copies to
/tmp/kindnet.specand/tmp/topolvm.specplus the asset/dropin/greenboot destinations line up with the described spec‑merge packaging approach; I don’t see path or ownership issues here.docs/run.md (3)
9-21: Optional package matrix is clear and matches the networking/storage story.The table succinctly explains Kindnet vs OVN‑K and TopoLVM/OLM; the note that one of
microshift-kindnetormicroshift-networkingmust be installed makes the dependency explicit.
70-84: Service start/postinstall split is sensible across RPM and DEB.Having a dedicated
postinstall.shstep for RPM-based systems (and explicitly skipping it on Ubuntu) followed by a genericsystemctl start microshift.servicekeeps the host setup clear and avoids leaking RPM-specific tweaks into the DEB path.
22-37: Local RPM installation flow requires manual verification due to inaccessible repository.The documented flow using
create_repos.sh -create,dnf install, andcreate_repos.sh -deletecannot be verified against the actual script implementations. While the approach appears coherent in theory, confirmation thatcreate_repos.shbehaves as documented and that the-createand-deleteflags function correctly requires access to the actual script code and related documentation.
pmtk
force-pushed
the
coprx
branch
2 times, most recently
from
b544106 to
c5c9d06
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
docs/run.md (2)
43-43: Fix COPR repository namespace — still incorrect from previous review.The COPR repository namespace is
@redhat-et/microshift, not@microshift-io/microshift. Update all references:-sudo dnf copr enable @microshift-io/microshift +sudo dnf copr enable @redhat-et/microshift-sudo dnf copr enable @microshift-io/microshift epel-9-x86_64 -sudo dnf copr enable @microshift-io/microshift epel-9-aarch64 -sudo dnf copr enable @microshift-io/microshift fedora-42-x86_64 -sudo dnf copr enable @microshift-io/microshift fedora-42-aarch64 +sudo dnf copr enable @redhat-et/microshift epel-9-x86_64 +sudo dnf copr enable @redhat-et/microshift epel-9-aarch64 +sudo dnf copr enable @redhat-et/microshift fedora-42-x86_64 +sudo dnf copr enable @redhat-et/microshift fedora-42-aarch64Also applies to: 48-51
66-66: Pass parent RPM directory toinstall.sh, not thedeb/subdirectory.The
install.shscript expects the parent RPM directory and uses recursivefindcommands to locate .deb files in subdirectories. Based on learnings from previous reviews:-DEB_REPO_DIR=/tmp/microshift-rpms/deb +DEB_REPO_DIR=/tmp/microshift-rpmssrc/image/modify-spec.py (1)
82-85: Magic indexlen(sections) - 3remains fragile.The prior review comment about this fragile insertion point was not addressed. The code still relies on a fixed offset from the end, which will break silently if the specfile structure changes. Consider searching for a named anchor (e.g., the changelog section) instead.
- # Add before the section that preceeds the changelog to keep the changelog 'usage' comment in right place - print(f"Adding section: '{extra_section.id}' to MicroShift downstream specfile") - sections.insert(len(sections) - 3, extra_section) + # Find the changelog section and insert before it + changelog_idx = next((i for i, s in enumerate(sections) if s.id == 'changelog'), len(sections)) + if changelog_idx == len(sections): + print(f"WARNING: Changelog section not found, appending section: '{extra_section.id}'") + else: + print(f"Adding section: '{extra_section.id}' before changelog") + sections.insert(changelog_idx, extra_section)
🧹 Nitpick comments (3)
packaging/rpms-copr.Containerfile (1)
8-15: Consider validatingCOPR_BUILD_IDbefore use.If
COPR_BUILD_IDis empty, thecopr download-buildcommand will fail with a cryptic error. Adding an explicit check would improve debuggability.ARG COPR_BUILD_ID= ARG BUILDER_RPM_REPO_PATH=/home/microshift/microshift/_output/rpmbuild/RPMS SHELL ["/bin/bash", "-o", "pipefail", "-c"] # hadolint ignore=DL3003 RUN \ + if [ -z "${COPR_BUILD_ID}" ]; then \ + echo "ERROR: COPR_BUILD_ID is not set" >&2; \ + exit 1; \ + fi && \ copr download-build --rpms --chroot "epel-9-$(uname -m)" --dest /tmp/rpms ${COPR_BUILD_ID} && \src/image/modify-spec.py (1)
38-43: Optional: Use iterable unpacking for concatenation.Static analysis suggests using iterable unpacking instead of list concatenation for slightly better readability.
-install_keywords_to_remove = pkgs_to_remove + [ +install_keywords_to_remove = [ + *pkgs_to_remove, 'lib/tuned', '05-high-performance-runtime.conf', 'microshift-baseline',src/copr/copr.mk (1)
72-79: Confirm whethercopr-watch-buildneeds COPR config/secretUnlike the other COPR CLI targets,
copr-watch-builddoes not inject thecopr-cfgsecret or pass--config, so it will only work ifcopr-cli watch-buildcan operate anonymously with just the build ID (or if the CLI image bakes in config). If your COPR project or API usage requires authentication to poll builds, consider aligning this with the other targets:-.PHONY: copr-watch-build -copr-watch-build: copr-cli +.PHONY: copr-watch-build +copr-watch-build: copr-cfg-ensure-podman-secret copr-cli @echo "Watching the COPR build" sudo podman run \ --rm \ - --volume "${SRPM_WORKDIR}:/srpms:Z" \ - "${COPR_CLI_IMAGE}" \ - bash -c "copr-cli watch-build \$$(cat /srpms/build.txt)" + --secret ${COPR_SECRET_NAME} \ + --volume "${SRPM_WORKDIR}:/srpms:Z" \ + "${COPR_CLI_IMAGE}" \ + bash -c "copr-cli --config /run/secrets/copr-cfg watch-build \$$(cat /srpms/build.txt)"Please double-check against your COPR project setup and CLI expectations.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.github/actions/build/action.yaml(3 hunks).github/actions/prebuild/action.yaml(1 hunks).github/workflows/release.md(1 hunks).github/workflows/release.yaml(3 hunks)Makefile(4 hunks)docs/run.md(2 hunks)docs/workflows.md(2 hunks)packaging/microshift-builder.Containerfile(1 hunks)packaging/microshift-runner.Containerfile(1 hunks)packaging/rpms-copr.Containerfile(1 hunks)packaging/srpm.Containerfile(1 hunks)src/copr/copr-cli.Containerfile(1 hunks)src/copr/copr.mk(1 hunks)src/copr/create-build.sh(1 hunks)src/image/build-rpms.sh(1 hunks)src/image/modify-spec.py(4 hunks)src/image/prebuild.sh(1 hunks)src/quickrpm.sh(3 hunks)src/rpm/create_repos.sh(3 hunks)src/topolvm/topolvm.spec(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- .github/workflows/release.md
- src/rpm/create_repos.sh
- .github/actions/build/action.yaml
- packaging/microshift-runner.Containerfile
- src/image/prebuild.sh
- packaging/srpm.Containerfile
- src/copr/create-build.sh
- src/topolvm/topolvm.spec
- .github/actions/prebuild/action.yaml
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-26T06:46:33.353Z
Learnt from: ggiguash
Repo: microshift-io/microshift PR: 137
File: src/quickrpm.sh:83-98
Timestamp: 2025-11-26T06:46:33.353Z
Learning: In the microshift-io/microshift repository, the quickrpm.sh script only supports RHEL 9 or above (and equivalent CentOS/Fedora versions), where util-linux includes the `--nooverlap` flag for losetup.
Applied to files:
packaging/rpms-copr.Containerfilesrc/quickrpm.shsrc/image/build-rpms.shdocs/workflows.mddocs/run.mdpackaging/microshift-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:
packaging/rpms-copr.Containerfilesrc/quickrpm.shsrc/image/build-rpms.shdocs/workflows.mddocs/run.md.github/workflows/release.yamlpackaging/microshift-builder.ContainerfileMakefilesrc/image/modify-spec.py
📚 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/quickrpm.shsrc/image/build-rpms.shdocs/run.mdpackaging/microshift-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:
src/copr/copr.mkdocs/workflows.md.github/workflows/release.yamlpackaging/microshift-builder.ContainerfileMakefile
🪛 Ruff (0.14.7)
src/image/modify-spec.py
38-43: Consider iterable unpacking instead of concatenation
Replace with iterable unpacking
(RUF005)
🔇 Additional comments (14)
docs/run.md (3)
26-37: Local RPMs installation block looks good.Clear instructions with proper repository creation and cleanup.
59-68: DEB installation section is well-documented once the path issue is fixed.Provides necessary context and links to build documentation.
70-93: Start MicroShift Service section is well-organized.Clear separation of postinstall configuration (RPM-only) and service startup, with verification steps appropriately placed.
docs/workflows.md (1)
54-57: Clear workflow description with appropriate SRPM and COPR context.The updated description accurately conveys that the workflow now produces SRPM packages and uses COPR for RPM builds, which aligns well with the PR objectives. The artifact list is consistent with the revised release process.
src/quickrpm.sh (1)
8-8: LGTM! COPR integration is well-implemented.The COPR enablement flow correctly replaces the previous GitHub release tarball approach. Error handling for missing
minor_versionaddresses the prior concern, and the repo name transformation logic is sound.Also applies to: 62-92
.github/workflows/release.yaml (2)
224-233: COPR repository regeneration ensures fresh metadata.The regenerate step correctly refreshes COPR repository metadata after artifacts are released, which is essential for downstream consumers.
119-122: Verify whether the build action explicitly validates"not-required"as a sentinel value.The GitHub Actions platform does not automatically enforce input validation—it initializes all missing inputs to empty strings, and you cannot reliably distinguish "not provided" from "unset" without explicit code-level validation. Passing literal
"not-required"values requires the downstream build action to have clear logic that recognizes and handles this sentinel. Confirm that.github/actions/build/action.yamlincludes explicit validation for these inputs whenrpm-builder: rpm-copr-builderis set, or consider using a documented standard pattern (e.g., empty string, a documented sentinel like__UNSET__, or a conditional input in the action metadata).src/copr/copr-cli.Containerfile (1)
1-3: LGTM! Clean, minimal COPR CLI container.src/image/build-rpms.sh (1)
74-82: Correct placement of version file writes.Version files are now written only for their respective build types (rpm vs srpm), preventing incorrect version propagation.
packaging/microshift-builder.Containerfile (1)
52-75: Staged spec-file workflow improves maintainability.The refactored approach—copying specs to temporary locations, then merging via
modify-spec.py—is cleaner and more composable than the previous inline approach.Makefile (3)
25-31: Architecture-specific OKD release images correctly configured.The conditional selection based on
$(ARCH)properly routes to architecture-specific release images.
33-33: OverridableBUILDER_IMAGEenables COPR workflow.Making
BUILDER_IMAGEoverridable (line 33) and updating the error message (lines 112-113) correctly supports both traditionalrpmand newrpm-coprbuild paths.Also applies to: 112-113
85-97: Newsrpmtarget is well-structured.The SRPM build target correctly parameterizes architecture-specific release images and output directory handling.
src/copr/copr.mk (1)
1-25: rpm-copr flow and RPM extraction look solidThe builder image tagging, use of
COPR_BUILD_IDas a build-arg, and the mounted-image copy ofRPMSinto a configurableRPM_OUTDIRdirectory are coherent and well-quoted. This target should be easy to script around for consumers.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
docs/run.md (2)
43-43: Fix incorrect COPR repository namespace (unresolved from past review).The COPR repository namespace is
@redhat-et/microshift, not@microshift-io/microshift. Users following these docs will enable the wrong repository. Update all occurrences:-sudo dnf copr enable @microshift-io/microshift +sudo dnf copr enable @redhat-et/microshift-sudo dnf copr enable @microshift-io/microshift epel-9-x86_64 -sudo dnf copr enable @microshift-io/microshift epel-9-aarch64 -sudo dnf copr enable @microshift-io/microshift fedora-42-x86_64 -sudo dnf copr enable @microshift-io/microshift fedora-42-aarch64 +sudo dnf copr enable @redhat-et/microshift epel-9-x86_64 +sudo dnf copr enable @redhat-et/microshift epel-9-aarch64 +sudo dnf copr enable @redhat-et/microshift fedora-42-x86_64 +sudo dnf copr enable @redhat-et/microshift fedora-42-aarch64Also applies to: 48-51
66-67: Pass parent directory toinstall.sh, not thedeb/subdirectory.The
install.shscript expects the parent RPM directory and uses recursivefindcommands to locate .deb files in subdirectories. Update the path:-DEB_REPO_DIR=/tmp/microshift-rpms/deb +DEB_REPO_DIR=/tmp/microshift-rpms
🧹 Nitpick comments (1)
.github/workflows/release.yaml (1)
49-59: Explicitly clean up COPR_CONFIG file after use.Lines 54 and 230 write the
COPR_CONFIGsecret to/tmp/copr-configwithout explicit cleanup. While the CI environment is ephemeral and/tmpwill be cleared on shutdown, it's a better practice to clean up sensitive files immediately after use.Apply this diff to add explicit cleanup:
run: | set -euo pipefail cd ${GITHUB_WORKSPACE}/ echo "${COPR_CONFIG}" > /tmp/copr-config + trap "rm -f /tmp/copr-config" EXIT make copr-create-build \ SRPM_WORKDIR=/mnt/srpm \ COPR_REPO_NAME="${{ inputs.copr-repo }}" \ COPR_CONFIG=/tmp/copr-configAlso applies to: 227-233
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.github/workflows/release.yaml(3 hunks)docs/run.md(2 hunks)packaging/srpm.Containerfile(1 hunks)src/copr/copr.mk(1 hunks)src/copr/create-build.sh(1 hunks)src/quickrpm.sh(3 hunks)src/rpm/create_repos.sh(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/rpm/create_repos.sh
- packaging/srpm.Containerfile
- src/copr/create-build.sh
- src/copr/copr.mk
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-11-26T06:46:33.353Z
Learnt from: ggiguash
Repo: microshift-io/microshift PR: 137
File: src/quickrpm.sh:83-98
Timestamp: 2025-11-26T06:46:33.353Z
Learning: In the microshift-io/microshift repository, the quickrpm.sh script only supports RHEL 9 or above (and equivalent CentOS/Fedora versions), where util-linux includes the `--nooverlap` flag for losetup.
Applied to files:
src/quickrpm.shdocs/run.md
📚 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/quickrpm.shdocs/run.md
📚 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/quickrpm.shdocs/run.md
📚 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/workflows/release.yaml
⏰ 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). (9)
- GitHub Check: ubuntu-rpm2deb
- GitHub Check: isolated-network (ovnk, ubuntu-24.04, 1)
- GitHub Check: isolated-network (kindnet, ubuntu-24.04, 0)
- GitHub Check: fedora-bootc (ubuntu-24.04-arm)
- GitHub Check: fedora-bootc (ubuntu-24.04)
- GitHub Check: centos9-bootc
- GitHub Check: centos10-bootc
- GitHub Check: quick-start-and-clean (ubuntu-24.04-arm)
- GitHub Check: quick-start-and-clean (ubuntu-24.04)
🔇 Additional comments (7)
src/quickrpm.sh (4)
8-8: LGTM: COPR variable addition.Default value prevents unintended empty repo references in typical usage.
62-62: LGTM: COPR repo enablement.Straightforward switch from tarball download to COPR-based repo.
72-89: LGTM: COPR repo_name transformation and minor_version extraction with validation.Repo_name transformation logic is correct:
@microshift-io/microshift→group_microshift-io:microshift, prefixed withcopr:copr.fedorainfracloud.org:. The error check for emptyminor_version(lines 83–86) properly addresses the previous issue flagged in PR review and fails fast with a clear diagnostic message.
72-89: Verify COPR repo_name format compatibility.Confirm that the constructed
repo_nameformat (copr:copr.fedorainfracloud.org:…) matches whatdnf --enablerepoexpects for COPR repos, especially for group-based repos (prefixgroup_). If test coverage exists for this flow, verify it passes..github/workflows/release.yaml (3)
159-161: Clarify why RPM archives are excluded from GitHub release.Line 138 creates RPM tarballs at
/mnt/release/microshift-rpms-*.tgz, but line 159–160 only releases DEB packages. Since COPR is now the primary RPM distribution channel, this exclusion is likely intentional, but it should be documented or confirmed. Either:
- Add a comment explaining that RPMs are distributed via COPR only, or
- If RPMs should be released to GitHub, add them to the
files:list.
37-44: Verify environment variable expansion in make target.Line 43 uses a ternary expression to choose between
inputs.okd-version-tagand the detected version. Confirm that themake srpmtarget correctly receives and processes these variables. No issues detected, but verify thatSRPM_WORKDIR=/mnt/srpmis writable in the build environment.
119-122: Verify required action inputs are properly satisfied.Lines 119–122 and 145–146 pass
"not-required"as string literals forushift-gitrefandokd-version-tagto the./.github/actions/buildand./.github/actions/build-debactions. Past reviews flagged this pattern as a critical issue. Confirm whether:
- The actions explicitly accept
"not-required"as a valid value (check action.yaml input definitions and their usage within action scripts), or- These inputs should be wired to actual values from workflow context (e.g., extracted from downloaded artifacts in earlier jobs).
Without access to the action definitions, this cannot be definitively verified. Provide the
inputs:sections from both action.yaml files to determine if the string literal is handled or if the inputs need to be properly resolved.Also applies to: 145–146
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/quickrpm.sh (1)
8-8: Confirm COPR parameter documentation.Line 8 introduces
COPRas a global variable with a sensible default. Ensure this is documented in the script's help/usage section or in external docs so users know they can override it (e.g.,COPR="user/project" ./quickrpm.sh).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/quickrpm.sh(3 hunks)
🧰 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:
src/quickrpm.sh
📚 Learning: 2025-11-26T06:46:33.353Z
Learnt from: ggiguash
Repo: microshift-io/microshift PR: 137
File: src/quickrpm.sh:83-98
Timestamp: 2025-11-26T06:46:33.353Z
Learning: In the microshift-io/microshift repository, the quickrpm.sh script only supports RHEL 9 or above (and equivalent CentOS/Fedora versions), where util-linux includes the `--nooverlap` flag for losetup.
Applied to files:
src/quickrpm.sh
📚 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/quickrpm.sh
⏰ 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). (9)
- GitHub Check: isolated-network (ovnk, ubuntu-24.04, 1)
- GitHub Check: isolated-network (kindnet, ubuntu-24.04, 0)
- GitHub Check: fedora-bootc (ubuntu-24.04-arm)
- GitHub Check: fedora-bootc (ubuntu-24.04)
- GitHub Check: centos10-bootc
- GitHub Check: ubuntu-rpm2deb
- GitHub Check: centos9-bootc
- GitHub Check: quick-start-and-clean (ubuntu-24.04)
- GitHub Check: quick-start-and-clean (ubuntu-24.04-arm)
🔇 Additional comments (3)
src/quickrpm.sh (3)
62-66: Verify dnf5 copr plugin installation is sufficient.The conditional installation of
dnf5-command(copr)handles the case where the copr plugin is missing from fedora-bootc. However, verify that:
- dnf (non-5) systems already have the copr plugin available by default
- This approach is compatible with all target OS versions (RHEL 9+, CentOS Stream, Fedora)
If you need confidence here, consider checking the base images or Fedora/RHEL package listings.
78-92: COPR repo name transformation and version retrieval with proper error handling.The logic correctly transforms COPR references (
@group/projectanduser/project) to the COPR repo name format and retrieves the minor version with an explicit error exit if the version cannot be determined. This addresses the prior review concern about handling emptyminor_version.
95-95: Verify create_repos.sh interface compatibility.The change from
-createto-deps-only "${minor_version}"indicates a shift in how the RPM repository is created. Confirm that:
create_repos.shaccepts the-deps-onlyflag with a minor version argument- The behavior produces the expected RPM repository setup (e.g., no unintended side effects from removing the
-createstep)Without access to
create_repos.shin this review, this warrants a quick verification against the actual script.
pmtk
force-pushed
the
coprx
branch
2 times, most recently
from
089a812 to
ecc1197
Compare
1 participant
Closes #71
Summary by CodeRabbit
New Features
Documentation
Chores
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.