CLID-621: simplify test catalog building#1420
Merged
Merged
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@r4f4: This pull request references CLID-621 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThis PR replaces the Go-based catalog generator with Makefile-driven pattern rules to discover, build, and push OPM-based catalog images; adds a unified catalog Dockerfile and README; and introduces four sets of test catalog fixtures (latest, diff, prune, prune-diff) with bundle/channel/operator YAMLs. ChangesCatalog build pipeline refactoring and test fixtures
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tests/integration/image-builders/operator/catalogs/catalog.Dockerfile (1)
5-5: ⚡ Quick winPrefer COPY over ADD for local files.
The
ADDinstruction is intended for URLs and tar archives. For copying local files and directories,COPYis the recommended best practice.♻️ Proposed fix
-ADD ${CATALOG} /configs +COPY ${CATALOG} /configs🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/image-builders/operator/catalogs/catalog.Dockerfile` at line 5, Replace the Dockerfile ADD instruction that copies local files (the line containing "ADD ${CATALOG} /configs") with the recommended COPY instruction; update the line to use "COPY ${CATALOG} /configs" so local directory content is copied without ADD's extra semantics, and keep the same build context and variable usage.tests/integration/image-builders/operator/Makefile (1)
6-7: 💤 Low valueConsider simplifying the target name construction.
The current
substusage works but is more complex than needed. Usingaddprefixwould be clearer and more idiomatic.♻️ Simpler alternative using addprefix
-CATALOG_TARGETS := $(foreach DIR,$(CATALOGDIRS), $(subst $(DIR),build-catalog.$(DIR),$(DIR))) -CATALOG_PUSH_TARGETS := $(foreach DIR,$(CATALOGDIRS), $(subst $(DIR),push-catalog.$(DIR),$(DIR))) +CATALOG_TARGETS := $(addprefix build-catalog.,$(CATALOGDIRS)) +CATALOG_PUSH_TARGETS := $(addprefix push-catalog.,$(CATALOGDIRS))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/image-builders/operator/Makefile` around lines 6 - 7, The target-name construction using subst is more complex than necessary: replace the two lines that define CATALOG_TARGETS and CATALOG_PUSH_TARGETS (which currently use $(foreach DIR,$(CATALOGDIRS), $(subst $(DIR),build-catalog.$(DIR),$(DIR))) and the analogous push variant) with simpler addprefix usage that prefixes each entry from CATALOGDIRS with "build-catalog." and "push-catalog." respectively (i.e., use addprefix to produce build-catalog.<dir> and push-catalog.<dir> from CATALOGDIRS so the variables CATALOG_TARGETS and CATALOG_PUSH_TARGETS are clearer and more idiomatic).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/image-builders/operator/catalogs/README.md`:
- Line 104: The mkdir invocation creates an unnecessary baz directory for the
test-catalog-prune-diff catalog; update the command that creates package dirs
(the line containing mkdir -p ${CATALOG}/{foo,bar,baz}) to only create the
actual packages used (remove "baz" so it becomes mkdir -p ${CATALOG}/{foo,bar})
to match the catalog contents defined earlier.
- Line 47: Update the documented baz bundle versions to match the versions
produced by the build/render command: locate the "baz: v0.1.0, v0.2.0, v1.0.0"
entry in the README and change it to the exact versions referenced by the opm
render command (the one on line with the `opm render` invocation) so the listed
bundles match the rendered/built outputs.
- Line 16: Update the README entry for the "baz" bundle versions so they match
the actual build command used by opm render; replace the incorrect list "v0.1.0,
v0.2.0, v1.0.0" with "v1.0.0, v1.0.1, v1.1.0" to reflect the bundles referenced
by the opm render command on line with that invocation.
In
`@tests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/operator.yaml`:
- Line 2: The operator layer's defaultChannel value ("defaultChannel" in
operator.yaml) does not match the intended layer channel scheme; verify the
intended channel (stable/alpha or "beta") and make them consistent by either
changing the defaultChannel in operator.yaml to the expected channel name (e.g.,
"stable" or "alpha") or updating the channels definition in channels.yaml to
include and treat "beta" as the intended channel; ensure the "beta" channel
entry in channels.yaml matches the name used in defaultChannel so they are
identical.
In `@tests/integration/image-builders/operator/Makefile`:
- Line 48: The .PHONY declaration currently lists "push-catalog" but the actual
Makefile target is "push-catalogs"; update the .PHONY line to include the
correct target name "push-catalogs" so the push-catalogs target is treated as
phony (i.e., replace or add push-catalogs to the .PHONY list to match the target
name used for push-catalogs).
---
Nitpick comments:
In `@tests/integration/image-builders/operator/catalogs/catalog.Dockerfile`:
- Line 5: Replace the Dockerfile ADD instruction that copies local files (the
line containing "ADD ${CATALOG} /configs") with the recommended COPY
instruction; update the line to use "COPY ${CATALOG} /configs" so local
directory content is copied without ADD's extra semantics, and keep the same
build context and variable usage.
In `@tests/integration/image-builders/operator/Makefile`:
- Around line 6-7: The target-name construction using subst is more complex than
necessary: replace the two lines that define CATALOG_TARGETS and
CATALOG_PUSH_TARGETS (which currently use $(foreach DIR,$(CATALOGDIRS), $(subst
$(DIR),build-catalog.$(DIR),$(DIR))) and the analogous push variant) with
simpler addprefix usage that prefixes each entry from CATALOGDIRS with
"build-catalog." and "push-catalog." respectively (i.e., use addprefix to
produce build-catalog.<dir> and push-catalog.<dir> from CATALOGDIRS so the
variables CATALOG_TARGETS and CATALOG_PUSH_TARGETS are clearer and more
idiomatic).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 621bcd99-0471-4cbd-b96c-7fde77f6cdf2
📒 Files selected for processing (34)
tests/integration/image-builders/operator/Makefiletests/integration/image-builders/operator/catalogs/README.mdtests/integration/image-builders/operator/catalogs/catalog.Dockerfiletests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/foo/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/foo/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/foo/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/baz/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/baz/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/baz/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/foo/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/foo/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/foo/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/operator.yamltests/integration/image-builders/operator/cmd/build-catalogs/main.go
💤 Files with no reviewable changes (1)
- tests/integration/image-builders/operator/cmd/build-catalogs/main.go
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/integration/image-builders/operator/catalogs/catalog.Dockerfile (1)
5-5: ⚡ Quick winPrefer
COPYoverADDfor local catalog content.
ADDis broader than needed here and can introduce unintended behavior;COPYis explicit for local paths.Suggested change
-ADD ${CATALOG} /configs +COPY ${CATALOG} /configs🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/image-builders/operator/catalogs/catalog.Dockerfile` at line 5, Replace the Dockerfile ADD instruction that copies the build-time variable ${CATALOG} into /configs with COPY to avoid ADD's extra behaviors; update the line using the same build variable (the symbol ${CATALOG}) so it becomes COPY ${CATALOG} /configs, ensuring we only copy local files and not trigger tar auto-extraction or remote fetch semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/image-builders/operator/catalogs/catalog.Dockerfile`:
- Line 3: Update the catalog.Dockerfile to pin the base image instead of using
the mutable tag and replace ADD with COPY: change the FROM line that currently
references quay.io/operator-framework/opm:latest to a digest-pinned reference
(quay.io/operator-framework/opm@sha256:...) to make builds reproducible, and
replace any ADD ${CATALOG} /configs usage with COPY ${CATALOG} /configs for
clarity; locate these edits in the catalog.Dockerfile where the FROM and ADD
${CATALOG} /configs entries appear.
In `@tests/integration/image-builders/operator/catalogs/README.md`:
- Line 32: The README's podman build command points to the wrong Dockerfile
path; update the command that references "../catalog.Dockerfile" so it uses the
Dockerfile in the current catalogs directory (e.g., "catalog.Dockerfile" or
"./catalog.Dockerfile") when invoking podman build with the CATALOG and -t
arguments so the build finds the correct file.
---
Nitpick comments:
In `@tests/integration/image-builders/operator/catalogs/catalog.Dockerfile`:
- Line 5: Replace the Dockerfile ADD instruction that copies the build-time
variable ${CATALOG} into /configs with COPY to avoid ADD's extra behaviors;
update the line using the same build variable (the symbol ${CATALOG}) so it
becomes COPY ${CATALOG} /configs, ensuring we only copy local files and not
trigger tar auto-extraction or remote fetch semantics.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c192a8d3-e8f2-4ca5-bf84-739b1cd114a4
📒 Files selected for processing (34)
tests/integration/image-builders/operator/Makefiletests/integration/image-builders/operator/catalogs/README.mdtests/integration/image-builders/operator/catalogs/catalog.Dockerfiletests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/foo/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/foo/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/foo/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/baz/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/baz/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/baz/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/foo/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/foo/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/foo/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/operator.yamltests/integration/image-builders/operator/cmd/build-catalogs/main.go
💤 Files with no reviewable changes (1)
- tests/integration/image-builders/operator/cmd/build-catalogs/main.go
✅ Files skipped from review due to trivial changes (18)
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/foo/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/foo/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/baz/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/bundles.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/bundles.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/bundles.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/bundles.yaml
🚧 Files skipped from review as they are similar to previous changes (11)
- tests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/bundles.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/foo/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/baz/bundles.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/baz/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/foo/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/bundles.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/bundles.yaml
- tests/integration/image-builders/operator/Makefile
| @@ -0,0 +1,11 @@ | |||
| ARG CATALOG="catalog" | |||
|
|
|||
| FROM quay.io/operator-framework/opm:latest | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show the target Dockerfile with line numbers
echo "=== Target Dockerfile ==="
nl -ba tests/integration/image-builders/operator/catalogs/catalog.Dockerfile | sed -n '1,120p'
# 2) Search for usage of mutable OPM tag(s)
echo "=== Search for opm:latest in repo ==="
rg -n "operator-framework/opm:latest" -S . || true
echo "=== Search for opm digests in repo ==="
rg -n "operator-framework/opm@sha256:" -S . || true
echo "=== Search for opm image usage in Dockerfiles ==="
rg -n "operator-framework/opm" -S --glob "**/*.Dockerfile" . || true
# 3) Check if there are other catalog/* Dockerfiles pinned similarly
echo "=== Similar Dockerfiles in same folder ==="
ls -la tests/integration/image-builders/operator/catalogs || true
find tests/integration/image-builders/operator/catalogs -maxdepth 2 -type f -name "*.Dockerfile" -print || trueRepository: openshift/oc-mirror
Length of output: 150
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Target Dockerfile ==="
cat -n tests/integration/image-builders/operator/catalogs/catalog.Dockerfile | sed -n '1,160p'
echo "=== Search for operator-framework/opm:latest in repo ==="
rg -n "operator-framework/opm:latest" -S . || true
echo "=== Search for operator-framework/opm@sha256: in repo ==="
rg -n "operator-framework/opm@sha256:" -S . || true
echo "=== Search for opm usage in Dockerfiles ==="
rg -n "operator-framework/opm" -S --glob "**/*.Dockerfile" . || true
echo "=== Similar Dockerfiles in same folder ==="
ls -la tests/integration/image-builders/operator/catalogs || true
find tests/integration/image-builders/operator/catalogs -maxdepth 2 -type f -name "*.Dockerfile" -print || trueRepository: openshift/oc-mirror
Length of output: 1595
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Show v1/test/operator/templates/Dockerfile ==="
cat -n v1/test/operator/templates/Dockerfile | sed -n '1,160p'
echo "=== Search for ADD ... /configs patterns ==="
rg -n "ADD\s+.*\s+/configs" -S --glob "*.Dockerfile" . || true
echo "=== Search for CATALOG build-arg usage in Dockerfiles ==="
rg -n "ARG\s+CATALOG" -S --glob "*.Dockerfile" . || true
rg -n "\$\{CATALOG\}" -S --glob "*.Dockerfile" . || trueRepository: openshift/oc-mirror
Length of output: 729
Pin quay.io/operator-framework/opm instead of using :latest in catalog.Dockerfile.
quay.io/operator-framework/opm:latest makes these integration catalog builds non-reproducible when the upstream image changes. Pin to a digest. Also consider switching ADD ${CATALOG} /configs to COPY ${CATALOG} /configs for clarity (another Dockerfile in this repo already uses COPY ... /configs).
Suggested change
+ARG OPM_IMAGE="quay.io/operator-framework/opm@sha256:<pinned-digest>"
-FROM quay.io/operator-framework/opm:latest
+FROM ${OPM_IMAGE}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/image-builders/operator/catalogs/catalog.Dockerfile` at
line 3, Update the catalog.Dockerfile to pin the base image instead of using the
mutable tag and replace ADD with COPY: change the FROM line that currently
references quay.io/operator-framework/opm:latest to a digest-pinned reference
(quay.io/operator-framework/opm@sha256:...) to make builds reproducible, and
replace any ADD ${CATALOG} /configs usage with COPY ${CATALOG} /configs for
clarity; locate these edits in the catalog.Dockerfile where the FROM and ADD
${CATALOG} /configs entries appear.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/integration/image-builders/operator/catalogs/catalog.Dockerfile (1)
3-3:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin the OPM base image to an immutable digest.
quay.io/operator-framework/opm:latestis mutable and makes test catalog builds non-reproducible across time. Please pin to a digest (or controlled tag+digest).#!/bin/bash set -euo pipefail # Verify current usage and whether any digest-pinned OPM image exists in repo. rg -n "quay.io/operator-framework/opm(:latest|`@sha256`:)" --glob "**/*.Dockerfile" .🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/image-builders/operator/catalogs/catalog.Dockerfile` at line 3, The Dockerfile's FROM uses the mutable image reference "quay.io/operator-framework/opm:latest"; replace that with an immutable digest-pinned reference (e.g. "quay.io/operator-framework/opm@sha256:<digest>" or a controlled tag+digest) so catalog builds are reproducible—update the FROM line in catalog.Dockerfile to the chosen digest-pinned image and verify the digest you use matches the intended OPM version.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/image-builders/operator/catalogs/catalog.Dockerfile`:
- Around line 7-9: The Dockerfile currently runs /bin/opm as root; add a
non-root user and switch to it before runtime by creating a user (e.g.,
"opmuser") and group, creating and chown/chmod'ing the cache directory so
/tmp/cache is writable by that user, then add a USER instruction so ENTRYPOINT
["/bin/opm"] and CMD ["serve", "/configs", "--cache-dir=/tmp/cache",
"--cache-only"] run as that non-root account; update any earlier RUN steps that
create /tmp/cache to set ownership/permissions for the new user and ensure no
later instruction reverts to root.
In `@tests/integration/image-builders/operator/catalogs/README.md`:
- Around line 19-31: Update the fenced code blocks in the README so each opening
triple-backtick includes a language identifier (use "bash"); specifically add
```bash before the blocks that start with "CATALOG=test-catalog-latest",
"CATALOG=test-catalog-diff", "CATALOG=test-catalog-prune",
"CATALOG=test-catalog-prune-diff", the block with "make build" / "make
build-catalog.<catalog-name>", and the block with "opm validate <catalog>" (and
the analogous blocks at the other locations referenced by the reviewer such as
lines containing CATALOG=test-catalog-diff, CATALOG=test-catalog-prune,
CATALOG=test-catalog-prune-diff, and the make/opm blocks). Leave the closing ```
unchanged.
---
Duplicate comments:
In `@tests/integration/image-builders/operator/catalogs/catalog.Dockerfile`:
- Line 3: The Dockerfile's FROM uses the mutable image reference
"quay.io/operator-framework/opm:latest"; replace that with an immutable
digest-pinned reference (e.g. "quay.io/operator-framework/opm@sha256:<digest>"
or a controlled tag+digest) so catalog builds are reproducible—update the FROM
line in catalog.Dockerfile to the chosen digest-pinned image and verify the
digest you use matches the intended OPM version.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 55afb47f-16b7-44ab-8a63-6602ca2d3c07
📒 Files selected for processing (34)
tests/integration/image-builders/operator/Makefiletests/integration/image-builders/operator/catalogs/README.mdtests/integration/image-builders/operator/catalogs/catalog.Dockerfiletests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/foo/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/foo/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/foo/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/baz/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/baz/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/baz/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/foo/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/foo/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/foo/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/operator.yamltests/integration/image-builders/operator/cmd/build-catalogs/main.go
💤 Files with no reviewable changes (1)
- tests/integration/image-builders/operator/cmd/build-catalogs/main.go
✅ Files skipped from review due to trivial changes (18)
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/foo/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/baz/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/bundles.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/bundles.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/foo/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/foo/bundles.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/bundles.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/bundles.yaml
🚧 Files skipped from review as they are similar to previous changes (13)
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/baz/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/foo/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/bundles.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/foo/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/bundles.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/baz/bundles.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/bundles.yaml
- tests/integration/image-builders/operator/Makefile
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/foo/bundles.yaml
Comment on lines
+7
to
+9
| ENTRYPOINT ["/bin/opm"] | ||
| CMD ["serve", "/configs", "--cache-dir=/tmp/cache", "--cache-only"] | ||
|
|
There was a problem hiding this comment.
Run the catalog image as non-root to satisfy DS-0002 and reduce container risk.
The image currently inherits root execution. Add a non-root USER and ensure /tmp/cache is writable by that user before opm serve.
Suggested patch
ARG CATALOG="catalog"
FROM quay.io/operator-framework/opm:latest
COPY ${CATALOG} /configs
ENTRYPOINT ["/bin/opm"]
CMD ["serve", "/configs", "--cache-dir=/tmp/cache", "--cache-only"]
+RUN mkdir -p /tmp/cache && chmod 0777 /tmp/cache
+USER 65532
# DC-specific label for the location of the DC root directory in the image
LABEL operators.operatorframework.io.index.configs.v1=/configs📝 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
| ENTRYPOINT ["/bin/opm"] | |
| CMD ["serve", "/configs", "--cache-dir=/tmp/cache", "--cache-only"] | |
| ARG CATALOG="catalog" | |
| FROM quay.io/operator-framework/opm:latest | |
| COPY ${CATALOG} /configs | |
| ENTRYPOINT ["/bin/opm"] | |
| CMD ["serve", "/configs", "--cache-dir=/tmp/cache", "--cache-only"] | |
| RUN mkdir -p /tmp/cache && chmod 0777 /tmp/cache | |
| USER 65532 | |
| # DC-specific label for the location of the DC root directory in the image | |
| LABEL operators.operatorframework.io.index.configs.v1=/configs |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/image-builders/operator/catalogs/catalog.Dockerfile` around
lines 7 - 9, The Dockerfile currently runs /bin/opm as root; add a non-root user
and switch to it before runtime by creating a user (e.g., "opmuser") and group,
creating and chown/chmod'ing the cache directory so /tmp/cache is writable by
that user, then add a USER instruction so ENTRYPOINT ["/bin/opm"] and CMD
["serve", "/configs", "--cache-dir=/tmp/cache", "--cache-only"] run as that
non-root account; update any earlier RUN steps that create /tmp/cache to set
ownership/permissions for the new user and ensure no later instruction reverts
to root.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adolfo-ab, r4f4 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
aguidirh
reviewed
May 29, 2026
Contributor
aguidirh
left a comment
aguidirh
left a comment
There was a problem hiding this comment.
Thanks for this PR, this is much better. It gives us much more control about the catalog we are using in our tests.
I added few comments. Let me know if you have questions about them.
Contributor
|
/assign @nidangavali This is an important one to learn, since our tests catalogs are going to be based on this new approach. |
Contributor
openshift-ci
Bot
added
the
do-not-merge/hold
Flatten out the test catalog config structure, so the catalog config is versioned instead of only being built at runtime. Added a README file with commands to rebuild catalogs, as well as their content.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/integration/image-builders/operator/catalogs/catalog.Dockerfile (2)
7-9:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a non-root USER directive to satisfy security requirements.
The catalog container runs as root, violating DS-0002. A previous comment suggested adding a non-root user and ensuring
/tmp/cacheis writable beforeopm serveexecutes.As per coding guidelines, container images should specify a non-root USER.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/image-builders/operator/catalogs/catalog.Dockerfile` around lines 7 - 9, The Dockerfile currently runs the catalog as root via ENTRYPOINT ["/bin/opm"] and CMD ["serve", "/configs", "--cache-dir=/tmp/cache", "--cache-only"]; add a non-root user and ensure /tmp/cache is writable before opm runs: create a user (e.g., group/user), create and chown /tmp/cache to that user at build time, and add a USER <username> directive before the ENTRYPOINT/CMD so that opm (the binary referenced by ENTRYPOINT) runs as the non-root user.
3-3:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPin the OPM base image to avoid non-reproducible builds.
Using
:latestmakes catalog builds non-reproducible when the upstream OPM image changes. This issue was previously flagged with a suggested fix to pin to a digest.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/image-builders/operator/catalogs/catalog.Dockerfile` at line 3, The Dockerfile currently uses an unpinned base image "quay.io/operator-framework/opm:latest", which makes builds non-reproducible; update the FROM line to pin the OPM image to a specific immutable identifier (either a stable version tag or, preferably, the image digest format quay.io/operator-framework/opm@sha256:<digest>), replacing "quay.io/operator-framework/opm:latest" with that pinned reference and, if relevant, add a short comment or CI step to document/rotate the digest when intentionally upgrading.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@tests/integration/image-builders/operator/catalogs/catalog.Dockerfile`:
- Around line 7-9: The Dockerfile currently runs the catalog as root via
ENTRYPOINT ["/bin/opm"] and CMD ["serve", "/configs", "--cache-dir=/tmp/cache",
"--cache-only"]; add a non-root user and ensure /tmp/cache is writable before
opm runs: create a user (e.g., group/user), create and chown /tmp/cache to that
user at build time, and add a USER <username> directive before the
ENTRYPOINT/CMD so that opm (the binary referenced by ENTRYPOINT) runs as the
non-root user.
- Line 3: The Dockerfile currently uses an unpinned base image
"quay.io/operator-framework/opm:latest", which makes builds non-reproducible;
update the FROM line to pin the OPM image to a specific immutable identifier
(either a stable version tag or, preferably, the image digest format
quay.io/operator-framework/opm@sha256:<digest>), replacing
"quay.io/operator-framework/opm:latest" with that pinned reference and, if
relevant, add a short comment or CI step to document/rotate the digest when
intentionally upgrading.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 38affd18-a49b-44a8-9ac8-52628afb9d63
📒 Files selected for processing (34)
tests/integration/image-builders/operator/Makefiletests/integration/image-builders/operator/catalogs/README.mdtests/integration/image-builders/operator/catalogs/catalog.Dockerfiletests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/foo/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/foo/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-diff/foo/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/baz/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/baz/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/baz/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/foo/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/foo/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/foo/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/operator.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/bundles.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/channels.yamltests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/operator.yamltests/integration/image-builders/operator/cmd/build-catalogs/main.go
💤 Files with no reviewable changes (1)
- tests/integration/image-builders/operator/cmd/build-catalogs/main.go
✅ Files skipped from review due to trivial changes (16)
- tests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/foo/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/bundles.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/baz/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/foo/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/foo/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/foo/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/bar/bundles.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/bundles.yaml
- tests/integration/image-builders/operator/catalogs/README.md
🚧 Files skipped from review as they are similar to previous changes (15)
- tests/integration/image-builders/operator/catalogs/test-catalog-prune/bar/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/baz/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune/foo/bundles.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/bundles.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/baz/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/operator.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/bar/channels.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-prune-diff/foo/bundles.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/bar/bundles.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-diff/foo/bundles.yaml
- tests/integration/image-builders/operator/catalogs/test-catalog-latest/foo/bundles.yaml
- tests/integration/image-builders/operator/Makefile
Contributor
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
@r4f4: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Contributor
|
/verified bypass |
openshift-ci-robot
added
the
verified
|
@aguidirh: The DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
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
5 participants
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.
Description
Flatten out the test catalog config structure, so the catalog config is versioned instead of only being built at runtime.
Added a README file with commands to rebuild catalogs, as well as their content.
Github / Jira issue: CLID-621
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration.
Expected Outcome
Please describe the outcome expected from the tests.
Summary by CodeRabbit