CLID-612: Move integration tests within oc-mirror#1400
Open
adolfo-ab wants to merge 3 commits intoopenshift:mainfrom
Open
CLID-612: Move integration tests within oc-mirror#1400adolfo-ab wants to merge 3 commits intoopenshift:mainfrom
adolfo-ab wants to merge 3 commits intoopenshift:mainfrom
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@adolfo-ab: This pull request references CLID-612 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 task 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. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a comprehensive Ginkgo-based integration test suite and supporting infrastructure: an oc-mirror CLI test runner, embedded registry helper, many integration test cases and helpers, test fixtures and build/builders, a container entrypoint, and a Makefile target to run integration tests (30m timeout). ChangesIntegration tests + test infra
sequenceDiagram
participant Test as Integration Test
participant Runner as oc-mirror Runner
participant Binary as oc-mirror (binary)
participant Registry as Embedded Registry
participant FS as Filesystem
Test->>Registry: Start(ctx, configPath, port)
Registry-->>Test: Ready (endpoint)
Test->>Runner: NewRunner(OC_MIRROR_BINARY)
Test->>Runner: MirrorToDisk / MirrorToMirror / DiskToMirror / List...
Runner->>Binary: exec.CommandContext(args, env)
Binary-->>Runner: stdout/stderr + exit code
Runner-->>Test: Result (ExitCode, Stdout, Stderr, Duration)
Test->>FS: inspect tar/mapping/IDMS files
Test->>Registry: ListRepositories / ListTags / IsCatalog
Registry-->>Test: repository/tag/catalog info
Test->>Registry: Stop()
Registry-->>Test: stopped, storage removed
🎯 3 (Moderate) | ⏱️ ~22 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
|
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: adolfo-ab The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (5)
tests/integration/helpers_test.go (2)
619-622: 💤 Low valuePrefix check on
configsPathcan match unintended paths.If
configsPathis e.g./configsand a tarred file is/configs-foo/bar,strings.HasPrefix(cleanPath, configsPath)returns true even thoughbarisn't under the FBC configs root. Anchor the comparison with a trailing separator (orfilepath.Rel+ check it doesn't escape with..).Proposed fix
- cleanPath := filepath.Clean("/" + hdr.Name) - if !strings.HasPrefix(cleanPath, configsPath) { - continue - } + cleanPath := filepath.Clean("/" + hdr.Name) + prefix := strings.TrimRight(configsPath, "/") + "/" + if cleanPath != configsPath && !strings.HasPrefix(cleanPath, prefix) { + continue + }🤖 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/helpers_test.go` around lines 619 - 622, The prefix check using strings.HasPrefix(cleanPath, configsPath) can falsely match paths like "/configs-foo"; update the validation around cleanPath (derived from hdr.Name) and configsPath to ensure the file is truly inside the configs root by either appending a path separator to configsPath before comparing (e.g. ensure configsPath has a trailing filepath.Separator and then check HasPrefix) or use filepath.Rel(configsPath, cleanPath) and reject if the result starts with ".." or is equal to "..", so only paths that are children of configsPath pass.
228-261: 💤 Low valueInconsistent
ctxhandling — mix of package-level variable and explicit parameter.Three helper functions (
expectRepositoriesExist,expectEmptyRegistry,expectNoRepositoriesInRegistry) reference a barectxpackage-level variable fromintegration_suite_test.go, whileexpectCatalogBundlesMatchISCandexpectRebuiltTagMatchesDigesttakectx context.Contextas an explicit parameter. Passingctxexplicitly to all helpers is more idiomatic Go and removes the hidden coupling to suite setup.🤖 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/helpers_test.go` around lines 228 - 261, The helpers expectRepositoriesExist, expectEmptyRegistry (and expectNoRepositoriesInRegistry) currently use a package-level ctx; change their signatures to accept ctx context.Context, update their internal calls (registry.ListRepositories(ctx), reg.ListTags(ctx, ...), reg.IsCatalog(ctx, ...)) to use the passed ctx, and update all call sites to pass the explicit ctx (matching how expectCatalogBundlesMatchISC and expectRebuiltTagMatchesDigest work) so there is no hidden coupling to the suite-level variable.tests/integration/pkg/ocmirror/runner.go (1)
28-35: 💤 Low valueDefault binary path silently depends on cwd.
"../../bin/oc-mirror"only resolves correctly when callers run fromtests/integration/. The newmake test-integration-clitarget doescd tests/integration, so this works today, but a futurego test ./...from the repo root or any IDE-driven run will fail with an opaque "no such file" fromexec. Consider resolving this path relative toruntime.Callerof this file, or returning an explicit error ifbinaryPathis empty.🤖 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/pkg/ocmirror/runner.go` around lines 28 - 35, NewRunner currently defaults binaryPath to a relative "../../bin/oc-mirror" which depends on the current working directory; update NewRunner to either (a) resolve that default path relative to this source file using runtime.Caller + filepath.Dir and filepath.Join so it reliably finds tests/integration/bin/oc-mirror regardless of cwd, or (b) change the signature of NewRunner(binaryPath string) to return (*Runner, error) and return an explicit error when binaryPath is empty, updating callers accordingly; reference the NewRunner function and the Runner.binaryPath field when making the change.tests/integration/pkg/registry/registry.go (1)
46-47: 💤 Low valueGlobal state mutation without restore.
os.Setenv("OTEL_TRACES_EXPORTER", "none")(error also ignored) andlogrus.SetOutput(io.Discard)permanently mutate process-wide state on everyStartcall. If anything in the same test binary depends on logrus output or the OTEL env var, it will be silently affected. Consider doing this once inTestMain/BeforeSuiterather than insideStart, and check theSetenverror.🤖 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/pkg/registry/registry.go` around lines 46 - 47, The Start function in tests/integration/pkg/registry/registry.go is mutating global process state by calling os.Setenv("OTEL_TRACES_EXPORTER", "none") (ignoring the error) and logrus.SetOutput(io.Discard"); change this so you either 1) move these global mutations out of Start into a test-wide setup (e.g., TestMain or BeforeSuite) to run once for the whole test binary, or 2) if they must live in Start, capture and check the error return from os.Setenv and save/restore the previous values (restore the original OTEL_TRACES_EXPORTER value and the original logrus output writer after the caller is done) so Start no longer leaves permanent side effects; refer to the Start function and the calls to os.Setenv and logrus.SetOutput to locate where to implement the fix.tests/integration/list_test.go (1)
141-151: ⚡ Quick win
ContainElementspermits extra output lines — confirm the loose-match semantics are intentional.
ContainElementsonly verifies that all elements inexpectedappear somewhere inlines; it passes even if the output contains additional unrelated entries. If the intent is strict equality (e.g., "these and only these versions are listed"), useConsistOfinstead.🤖 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/list_test.go` around lines 141 - 151, compareLineOutput uses Gomega's ContainElements which allows extra, unrelated lines to be present; replace the loose assertion to enforce a strict match. In the compareLineOutput function change the assertion Expect(lines).To(ContainElements(expected), ...) to a strict assertion such as Expect(lines).To(ConsistOf(expected), ...) if order doesn't matter, or Expect(lines).To(Equal(expected), ...) if order must match, keeping the existing header-skipping and trimming logic intact.
🤖 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/d2m_invalid_from_test.go`:
- Around line 41-45: The test's expectation in expectOcMirrorCommandInvalidFrom
uses a raw string literal with `ContainSubstring` that contains `\\.` which
becomes two literal backslashes and therefore never matches the oc-mirror output
(which contains a single backslash). Fix by replacing the raw/backtick string
with a regular double-quoted Go string for the ContainSubstring argument and
escape the backslash once (i.e., use `\\.` inside the double-quoted string) so
the matcher searches for a single `\.` as emitted by oc-mirror; update the
ContainSubstring call in expectOcMirrorCommandInvalidFrom accordingly.
In `@tests/integration/exit_codes_test.go`:
- Around line 53-63: The test "should return exit code 8 when additional images
fail to mirror" calls runner.MirrorToDisk and asserts exit code but omits the
usual expectNoTarArchive(workDir) check; add a call to
expectNoTarArchive(workDir) after expectOcMirrorExitCode (or, if a partial tar
archive is expected here, add a brief comment above the test explaining that
releases/operators may succeed and produce a partial archive) so the test
matches the pattern used by the other exit-code specs and explicitly documents
the intended behavior.
In `@tests/integration/go.mod`:
- Line 6: Update the direct dependency line for the module
github.com/distribution/distribution/v3 from v3.0.0 to v3.1.0 to remediate the
reported HIGH-severity CVEs, then run go mod tidy in the integration tests
module to refresh transitive pins in go.sum; ensure the updated go.mod
references v3.1.0 and that go.sum is checked in after tidying.
- Line 28: Update the indirect dependency for github.com/docker/cli to a patched
release (v29.2.0 or later) by changing the version in go.mod and then run go get
github.com/docker/cli@v29.2.0 (or higher) followed by go mod tidy to refresh
go.sum; ensure CI/builds pick up the new version and commit the updated go.mod
and go.sum.
In `@tests/integration/helpers_test.go`:
- Line 51: The TODO mentioning "Remove these structs once we move the
integration tests into the oc-mirror repo" is stale — replace the local test
structs with the canonical API type v2alpha1.ImageSetConfiguration from
internal/pkg/api (importing that package and switching uses of the local structs
to v2alpha1.ImageSetConfiguration) or, if you prefer to keep local copies
temporarily, update the TODO to clearly state the remaining action (for example
"Replace local test structs with internal/pkg/api v2alpha1.ImageSetConfiguration
once stable") and reference the local struct names used in tests so it's clear
what to change; locate usages in tests/integration/helpers_test.go and any
helpers that construct or assert on those types (e.g., helper constructors or
assertions) and update imports/usages accordingly.
- Around line 776-792: The version regex in isOCMirrorVersionBefore only allows
a single-digit major; update the regexp used in that function to capture
multi-digit majors (and preferably anchor the pattern) so versions like "10.5"
parse correctly (e.g. change `(\d)\.(\d+)` to a pattern that uses `\d+` and
start anchor); keep the remainder of the logic intact (use the same
parts[1]/parts[2] parsing and error checks) so verMajor and verMinor are
computed correctly for multi-digit majors.
- Around line 521-545: The defer os.RemoveAll(configsDir) inside
expectCatalogBundlesMatchISC delays cleanup until the whole function returns,
accumulating temp dirs and skipping cleanup on early test failures; fix by
scoping each operator iteration into its own closure or helper so the defer
executes at the end of that iteration (e.g. wrap the body that calls
extractCatalogConfigs, loadCatalogBundles and the Expect checks in an anonymous
func or extractOperatorCheck(op) helper and place defer os.RemoveAll(configsDir)
inside it), or alternatively call os.RemoveAll(configsDir) immediately after the
per-operator work; keep references to extractCatalogConfigs, loadCatalogBundles
and the Expect checks unchanged.
In `@tests/integration/list_test.go`:
- Around line 67-70: The skip message inside the BeforeAll block for the "list
releases" test suite is incorrect; update the Skip(...) call in the BeforeAll
function (in tests/integration/list_test.go) to a message that references
"oc-mirror list releases not available in < 4.22" or otherwise correctly
mentions "list releases" instead of "list operators" so the skip reason matches
the describe block.
- Around line 75-84: The mock HTTP handler currently writes the response body
before calling w.WriteHeader in the error branches (the Accept-type check and
the channel param check), which causes Go to implicitly send 200 OK; change the
order so the status is set before the body (call
w.WriteHeader(http.StatusNotAcceptable) / w.WriteHeader(http.StatusBadRequest)
before io.WriteString) or replace the branches with http.Error to send the
proper status and body for the Accept header check and the missing channel query
param check.
In `@tests/integration/pkg/registry/registry.go`:
- Line 43: The direct type assertion
config.Storage["filesystem"]["rootdirectory"].(string) can panic; update the
code that builds storagePath (referencing storagePath and config variables in
tests/integration/pkg/registry/registry.go) to validate presence and types
safely: first check config.Storage["filesystem"] exists and is a map (use a
type-assertion with ok), then check the "rootdirectory" key exists and is a
string (use the ok form), and if any check fails return a clear wrapped error
(e.g. fmt.Errorf("invalid registry config: %w", err)) instead of panicking; only
call filepath.Join when you have a validated string. Ensure the error message
names the missing/invalid key ("filesystem.rootdirectory") so test helpers
surface a useful error.
- Around line 54-71: The serve goroutine currently prints ListenAndServe errors
to stderr and lets WaitReady keep polling unaware; change this by storing the
served error on the Registry (e.g., add a serveErr error field or channel on
Registry and set it from the goroutine that calls service.ListenAndServe) and
update WaitReady to check that field/channel and return the serve error
immediately if present. Also ensure the registry client/Endpoint uses the actual
listen address: either parse the port from config.HTTP.Addr and use that when
calling name.NewRegistry and in Endpoint(), or override config.HTTP.Addr with
the provided port before starting the server and before constructing the
name.NewRegistry client so the port used by service.ListenAndServe,
name.NewRegistry(fmt.Sprintf("localhost:%d", port)...), and Endpoint() all
match. Ensure references: service.ListenAndServe goroutine, Registry struct (add
serveErr), WaitReady, name.NewRegistry call, and config.HTTP.Addr are updated
accordingly.
- Line 32: The Start function's variadic parameter logWriter ...interface{} is
unused and of the wrong type; either remove it or change it to io.Writer and
wire it into the registry logger. Update the Start(ctx context.Context,
configPath string, port int, logWriter ...interface{}) signature to accept
logWriter io.Writer (or remove the param), then in the Start implementation set
logrus.SetOutput(logWriter) or pass the io.Writer into the Registry construction
so the registry's logger uses it (referencing Start, logWriter, Registry and use
logrus.SetOutput or Registry logger initialization to apply the writer).
In `@tests/integration/testdata/registry-config.yaml`:
- Around line 13-16: The test config hardcodes storage.filesystem.rootdirectory
(/tmp/) and http.addr (:5000) which breaks isolation; modify the test setup so
Registry.Start (or the test helper that writes
tests/integration/testdata/registry-config.yaml) generates a unique temp
directory and selects a free port, then injects those values into
storage.filesystem.rootdirectory and http.addr before the registry parses/starts
(or write a templated config file per-test into a tempdir); ensure Registry.Stop
still cleans up the generated directory to avoid leaks and that the chosen port
is passed into the registry start routine to avoid collisions.
---
Nitpick comments:
In `@tests/integration/helpers_test.go`:
- Around line 619-622: The prefix check using strings.HasPrefix(cleanPath,
configsPath) can falsely match paths like "/configs-foo"; update the validation
around cleanPath (derived from hdr.Name) and configsPath to ensure the file is
truly inside the configs root by either appending a path separator to
configsPath before comparing (e.g. ensure configsPath has a trailing
filepath.Separator and then check HasPrefix) or use filepath.Rel(configsPath,
cleanPath) and reject if the result starts with ".." or is equal to "..", so
only paths that are children of configsPath pass.
- Around line 228-261: The helpers expectRepositoriesExist, expectEmptyRegistry
(and expectNoRepositoriesInRegistry) currently use a package-level ctx; change
their signatures to accept ctx context.Context, update their internal calls
(registry.ListRepositories(ctx), reg.ListTags(ctx, ...), reg.IsCatalog(ctx,
...)) to use the passed ctx, and update all call sites to pass the explicit ctx
(matching how expectCatalogBundlesMatchISC and expectRebuiltTagMatchesDigest
work) so there is no hidden coupling to the suite-level variable.
In `@tests/integration/list_test.go`:
- Around line 141-151: compareLineOutput uses Gomega's ContainElements which
allows extra, unrelated lines to be present; replace the loose assertion to
enforce a strict match. In the compareLineOutput function change the assertion
Expect(lines).To(ContainElements(expected), ...) to a strict assertion such as
Expect(lines).To(ConsistOf(expected), ...) if order doesn't matter, or
Expect(lines).To(Equal(expected), ...) if order must match, keeping the existing
header-skipping and trimming logic intact.
In `@tests/integration/pkg/ocmirror/runner.go`:
- Around line 28-35: NewRunner currently defaults binaryPath to a relative
"../../bin/oc-mirror" which depends on the current working directory; update
NewRunner to either (a) resolve that default path relative to this source file
using runtime.Caller + filepath.Dir and filepath.Join so it reliably finds
tests/integration/bin/oc-mirror regardless of cwd, or (b) change the signature
of NewRunner(binaryPath string) to return (*Runner, error) and return an
explicit error when binaryPath is empty, updating callers accordingly; reference
the NewRunner function and the Runner.binaryPath field when making the change.
In `@tests/integration/pkg/registry/registry.go`:
- Around line 46-47: The Start function in
tests/integration/pkg/registry/registry.go is mutating global process state by
calling os.Setenv("OTEL_TRACES_EXPORTER", "none") (ignoring the error) and
logrus.SetOutput(io.Discard"); change this so you either 1) move these global
mutations out of Start into a test-wide setup (e.g., TestMain or BeforeSuite) to
run once for the whole test binary, or 2) if they must live in Start, capture
and check the error return from os.Setenv and save/restore the previous values
(restore the original OTEL_TRACES_EXPORTER value and the original logrus output
writer after the caller is done) so Start no longer leaves permanent side
effects; refer to the Start function and the calls to os.Setenv and
logrus.SetOutput to locate where to implement the fix.
🪄 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: 3dbb09c2-350d-459c-9f64-05b4f9f2ed7a
⛔ Files ignored due to path filters (1)
tests/integration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
Makefiletests/integration/archive_test.gotests/integration/d2m_invalid_from_test.gotests/integration/delete_test.gotests/integration/dry_run_test.gotests/integration/exit_codes_test.gotests/integration/go.modtests/integration/helpers_test.gotests/integration/integration_suite_test.gotests/integration/list_test.gotests/integration/m2d_d2m_test.gotests/integration/m2m_test.gotests/integration/operators_test.gotests/integration/pkg/ocmirror/runner.gotests/integration/pkg/registry/registry.gotests/integration/testdata/graphdatas/graph_data.jsontests/integration/testdata/imagesetconfigs/happy_path/disc-happy-path.yamltests/integration/testdata/keys/release-pk.asctests/integration/testdata/keys/v0.0.1-sha256-f81792339c8b5934191d18a53b18bc1d584e01a9f37d59c0aa6905b00200aa1btests/integration/testdata/registry-config.yaml
adolfo-ab
force-pushed
the
add-integration-tests
branch
from
f73d26b to
7e7c5be
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
tests/integration/list_test.go (1)
75-84:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMock server error branches still respond HTTP 200 —
WriteHeadercalled afterWrite.
io.WriteString(w, ...)implicitly commits200 OKbefore thew.WriteHeader(...)call; Go emits a "superfluous response.WriteHeader call" warning and the status is ignored. Both error paths always return200 OK.🐛 Proposed fix using
http.Errorif contentType := r.Header.Get("Accept"); contentType != "application/json" { - io.WriteString(w, `{"kind":"invalid_content_type","value": "invalid Content-Type requested"}`) - w.WriteHeader(http.StatusNotAcceptable) + http.Error(w, `{"kind":"invalid_content_type","value": "invalid Content-Type requested"}`, http.StatusNotAcceptable) return } query := r.URL.Query() if query.Get("channel") == "" { - io.WriteString(w, `{"kind":"missing_params","value":"mandatory client parameters missing: channel"}`) - w.WriteHeader(http.StatusBadRequest) + http.Error(w, `{"kind":"missing_params","value":"mandatory client parameters missing: channel"}`, http.StatusBadRequest) return }🤖 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/list_test.go` around lines 75 - 84, The mock HTTP handler in tests/integration/list_test.go writes the response body with io.WriteString before calling w.WriteHeader, which causes a 200 OK to be sent and ignores the subsequent WriteHeader; change the error branches to set the status first (call w.WriteHeader(http.StatusNotAcceptable) and w.WriteHeader(http.StatusBadRequest) before writing the JSON) or replace the pair with http.Error(w, <json string>, http.StatusNotAcceptable/http.StatusBadRequest) so the correct non-200 status is sent; update the branches that use r.Header.Get("Accept") and query.Get("channel") accordingly.
🧹 Nitpick comments (3)
tests/integration/delete_test.go (2)
21-23: 💤 Low valueAvoid
:=assignments in Ginkgo container bodies for spec-scoped variables.
iscHappyPath,discHappyPath, anddeleteIdare initialised at tree-build time, not per-spec. For immutable string constants this is harmless today, but Ginkgo v2's guidelines recommend declaring these withvarin the container body and assigning them in aBeforeEachto prevent future contributors from accidentally adding mutable/stateful variables in the same style.♻️ Suggested refactor
Describe("delete functionality tests", func() { - iscHappyPath := filepath.Join("happy_path", "isc-happy-path.yaml") - discHappyPath := filepath.Join("happy_path", "disc-happy-path.yaml") - deleteId := "delete-test" + var ( + iscHappyPath string + discHappyPath string + deleteId string + ) + + BeforeEach(func() { + iscHappyPath = filepath.Join("happy_path", "isc-happy-path.yaml") + discHappyPath = filepath.Join("happy_path", "disc-happy-path.yaml") + deleteId = "delete-test" + })🤖 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/delete_test.go` around lines 21 - 23, The three spec-scoped variables iscHappyPath, discHappyPath and deleteId are using := in the Ginkgo container body (tree-build time); change them to package/local var declarations (e.g., var iscHappyPath, discHappyPath, deleteId string) and move their assignments into a BeforeEach that runs per-spec so values are set at spec runtime; update any references in the specs to use these vars and ensure the BeforeEach initializes the strings (and keeps them immutable) to follow Ginkgo v2 guidelines.
26-26: ⚡ Quick winHardcoded
deleteYamlpath is fragile ifDeletePhaseOne's output convention changes.The path
workDir/working-dir/delete/delete-images-<deleteId>.yamlis manually assembled from assumed knowledge ofDeletePhaseOne's output layout. If the subdirectory name, prefix, or extension ever changes,DeletePhaseTwowill silently receive a wrong/nonexistent path and fail with a confusing error rather than a clear path-mismatch failure. Consider extracting this construction into a shared helper (alongsideexpectValidDeleteImagesFiles) so both the assertion and the caller stay in sync automatically.🤖 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/delete_test.go` at line 26, The hardcoded construction of deleteYaml (deleteYaml := filepath.Join(workDir, "working-dir", "delete", "delete-images-"+deleteId+".yaml")) is brittle vs. DeletePhaseOne's output format; refactor by extracting a shared helper (e.g., constructDeleteImagesPath or GetDeleteImagesFile) used by both DeletePhaseTwo and the test helper expectValidDeleteImagesFiles so the path building logic lives in one place and can be updated centrally; update calls in tests and DeletePhaseTwo to use that helper and add a clear error if the file is missing.tests/integration/pkg/registry/registry.go (1)
52-54: 💤 Low value
logrus.SetOutput(io.Discard)permanently suppresses logrus output for the entire test process.This call is irreversible within the test binary lifetime — there is no per-
Registryrestore. If any other component (including oc-mirror helpers or other test packages) relies on logrus for diagnostic output, all of it is silently discarded from the moment the first registry is started. Consider scoping this to the time window betweenNewRegistryand when the service is ready, then restoring the original writer.♻️ Proposed fix
+ prevOutput := logrus.StandardLogger().Out logrus.SetOutput(io.Discard) service, err := dregistry.NewRegistry(ctx, config) + logrus.SetOutput(prevOutput) if err != nil { return nil, fmt.Errorf("failed to create registry: %w", err) }🤖 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/pkg/registry/registry.go` around lines 52 - 54, The call to logrus.SetOutput(io.Discard) permanently silences logrus for the entire process; instead capture the current output writer before calling health.NewRegistry (e.g., save original := logrus.StandardLogger().Out), set logrus output to io.Discard only for the short window where the registry is being created/initialized (around health.NewRegistry/health.DefaultRegistry and until the service is ready), then restore the original writer via logrus.SetOutput(original) as soon as initialization finishes; update the code that uses health.DefaultRegistry/health.NewRegistry and logrus.SetOutput(io.Discard) to perform this save-set-restore pattern so other tests/components keep their logs.
🤖 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 `@Makefile`:
- Around line 61-63: Add the missing phony declaration for the Makefile target
by including "test-integration-cli" in the .PHONY list so make doesn't treat a
same-named file/dir as up-to-date; update the existing .PHONY line (or add a
.PHONY block) to include the target name "test-integration-cli" alongside other
phony targets to ensure the "test-integration-cli" rule (the target that runs
tests/integration) is always executed.
In `@tests/integration/list_test.go`:
- Around line 132-139: The compareTableOutput helper can panic when result has
more data rows than expected; before iterating, assert the row counts match by
comparing len(lines)-1 (skip header) to len(expected) using Expect so failures
are reported by Gomega, then iterate safely (e.g., range over expected or the
known count) when calling parseFields and comparing to expected[idx]; update
compareTableOutput (referencing headerFields, result, expected, lines,
parseColumnIndexesFromHeader, parseFields) to perform this count assertion and
avoid direct out-of-bounds indexing.
- Around line 112-119: parseColumnIndexesFromHeader currently appends -1 when
strings.Index fails, which later causes runtime panics in parseFields; change
parseColumnIndexesFromHeader to detect idx == -1 and immediately fail with a
clear error (panic or testing.Fatalf) that includes the missing field name and
the header string so the test fails with a meaningful message, and add a
defensive check in parseFields to validate indexes before slicing (referencing
parseColumnIndexesFromHeader and parseFields).
- Around line 121-130: The parseFields function can panic for empty
headerIndexes and when line is shorter than expected; update parseFields to
first handle nIndexes==0 by returning an empty slice, then compute a safeLineLen
:= len(line) and for each pair of indices (using headerIndexes and nextIdx)
clamp nextIdx and idx to the range [0, safeLineLen] (or treat idx >= safeLineLen
as producing an empty trimmed field) so that slices like line[idx:nextIdx] never
go out of bounds, and likewise clamp or handle headerIndexes[nIndexes-1] when
producing the final field (e.g., return "" for fields that start beyond the line
length or pad the line virtually by treating missing chars as spaces). Ensure
you reference parseFields, headerIndexes, fields, idx and nextIdx when making
the changes.
- Around line 41-51: The test assumes lines from
slices.Collect(strings.Lines(result.Stdout)) always has >=4 entries and directly
slices lines[:2] and lines[3:], which can panic; update the test to guard on
len(lines) before slicing (e.g., check len(lines) >= 2 and >= 4) and if the
checks fail, fail the test with a clear assertion or error (using the same test
helpers/assertions used elsewhere) instead of slicing into an out-of-bounds
slice; apply these guards around the defChannelResult and chanResult
construction that use lines[:2] and lines[3:], and keep using compareTableOutput
for the validated cases.
In `@tests/integration/pkg/registry/registry.go`:
- Around line 44-50: The Stop() cleanup currently removes only the "docker"
subdirectory (storagePath) leaving the parent temp dir (storageRoot) behind;
update the cleanup in the Stop() method to remove the entire storageRoot created
by os.MkdirTemp (not just filepath.Join(storageRoot, "docker")), ensuring any
references to storagePath are cleaned first if necessary and then call
os.RemoveAll on storageRoot (use the same storageRoot variable created during
setup so both the docker subdir and the parent temp directory are removed).
---
Duplicate comments:
In `@tests/integration/list_test.go`:
- Around line 75-84: The mock HTTP handler in tests/integration/list_test.go
writes the response body with io.WriteString before calling w.WriteHeader, which
causes a 200 OK to be sent and ignores the subsequent WriteHeader; change the
error branches to set the status first (call
w.WriteHeader(http.StatusNotAcceptable) and w.WriteHeader(http.StatusBadRequest)
before writing the JSON) or replace the pair with http.Error(w, <json string>,
http.StatusNotAcceptable/http.StatusBadRequest) so the correct non-200 status is
sent; update the branches that use r.Header.Get("Accept") and
query.Get("channel") accordingly.
---
Nitpick comments:
In `@tests/integration/delete_test.go`:
- Around line 21-23: The three spec-scoped variables iscHappyPath, discHappyPath
and deleteId are using := in the Ginkgo container body (tree-build time); change
them to package/local var declarations (e.g., var iscHappyPath, discHappyPath,
deleteId string) and move their assignments into a BeforeEach that runs per-spec
so values are set at spec runtime; update any references in the specs to use
these vars and ensure the BeforeEach initializes the strings (and keeps them
immutable) to follow Ginkgo v2 guidelines.
- Line 26: The hardcoded construction of deleteYaml (deleteYaml :=
filepath.Join(workDir, "working-dir", "delete",
"delete-images-"+deleteId+".yaml")) is brittle vs. DeletePhaseOne's output
format; refactor by extracting a shared helper (e.g., constructDeleteImagesPath
or GetDeleteImagesFile) used by both DeletePhaseTwo and the test helper
expectValidDeleteImagesFiles so the path building logic lives in one place and
can be updated centrally; update calls in tests and DeletePhaseTwo to use that
helper and add a clear error if the file is missing.
In `@tests/integration/pkg/registry/registry.go`:
- Around line 52-54: The call to logrus.SetOutput(io.Discard) permanently
silences logrus for the entire process; instead capture the current output
writer before calling health.NewRegistry (e.g., save original :=
logrus.StandardLogger().Out), set logrus output to io.Discard only for the short
window where the registry is being created/initialized (around
health.NewRegistry/health.DefaultRegistry and until the service is ready), then
restore the original writer via logrus.SetOutput(original) as soon as
initialization finishes; update the code that uses
health.DefaultRegistry/health.NewRegistry and logrus.SetOutput(io.Discard) to
perform this save-set-restore pattern so other tests/components keep their logs.
🪄 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: 34fb81ad-dfe1-4dcc-b692-c004e1add239
⛔ Files ignored due to path filters (1)
tests/integration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (20)
Makefiletests/integration/archive_test.gotests/integration/d2m_invalid_from_test.gotests/integration/delete_test.gotests/integration/dry_run_test.gotests/integration/exit_codes_test.gotests/integration/go.modtests/integration/helpers_test.gotests/integration/integration_suite_test.gotests/integration/list_test.gotests/integration/m2d_d2m_test.gotests/integration/m2m_test.gotests/integration/operators_test.gotests/integration/pkg/ocmirror/runner.gotests/integration/pkg/registry/registry.gotests/integration/testdata/graphdatas/graph_data.jsontests/integration/testdata/imagesetconfigs/happy_path/disc-happy-path.yamltests/integration/testdata/keys/release-pk.asctests/integration/testdata/keys/v0.0.1-sha256-f81792339c8b5934191d18a53b18bc1d584e01a9f37d59c0aa6905b00200aa1btests/integration/testdata/registry-config.yaml
✅ Files skipped from review due to trivial changes (6)
- tests/integration/testdata/keys/release-pk.asc
- tests/integration/testdata/imagesetconfigs/happy_path/disc-happy-path.yaml
- tests/integration/testdata/registry-config.yaml
- tests/integration/testdata/graphdatas/graph_data.json
- tests/integration/go.mod
- tests/integration/m2d_d2m_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- tests/integration/d2m_invalid_from_test.go
- tests/integration/exit_codes_test.go
- tests/integration/operators_test.go
- tests/integration/integration_suite_test.go
- tests/integration/pkg/ocmirror/runner.go
- tests/integration/archive_test.go
- tests/integration/helpers_test.go
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (2)
tests/integration/image-builders/release/go.mod (1)
1-3: 💤 Low value
example.com/m/v2is a docs placeholder — consider a project-scoped path.Using the canonical Go docs placeholder as the real module path is harmless for a standalone test binary, but it's confusing if additional Go files are added to this subtree or if tooling/IDEs try to resolve it.
♻️ Suggested change
-module example.com/m/v2 +module github.com/openshift/oc-mirror/tests/integration/image-builders/release🤖 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/release/go.mod` around lines 1 - 3, Replace the placeholder module declaration "module example.com/m/v2" in the go.mod used by the image-builders/release test with a project-scoped module path so tooling and IDEs resolve correctly; edit the module line (the "module ..." entry in this go.mod) to a canonical repository-specific path (for example a github.com/your-org/your-repo/... style path that reflects this tests subtree) and run go mod tidy to ensure dependencies and module metadata remain valid.tests/integration/image-builders/release/create-release.sh (1)
11-12: ⚡ Quick win
podman inspectmay return the local manifest digest, not the registry digest.After
make push,podman inspect --format '{{.Digest}}'reflects the locally stored manifest digest. If the registry recompresses layers (e.g., converts to zstd), the registry digest will differ, causing a mismatch in the release manifests. Use--digestfileduring push to capture the authoritative registry digest.♻️ More reliable digest capture
In
Makefile, change thepushtarget:push: - podman push quay.io/oc-mirror/release/test-image:v0.0.1 --authfile=$(AUTHFILE) + podman push quay.io/oc-mirror/release/test-image:v0.0.1 --authfile=$(AUTHFILE) \ + --digestfile .pushed-digestThen in
create-release.sh:-TMP_DIGEST=$(podman inspect quay.io/oc-mirror/release/test-image:v0.0.1 | jq '.[].Digest') -DIGEST=$(echo "${TMP_DIGEST}" | tr -d '"') +DIGEST=$(cat .pushed-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/release/create-release.sh` around lines 11 - 12, The script currently derives the image digest via podman inspect (TMP_DIGEST/DIGEST) which can return the local manifest digest; update the image push to capture the authoritative registry digest by adding --digestfile to the push command in the Makefile's push target, and modify create-release.sh to read the registry digest from that digestfile (instead of using podman inspect TMP_DIGEST/DIGEST) so release manifests use the registry-provided digest.
🤖 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 `@images/cli/Dockerfile.test`:
- Line 9: Replace the insecure recursive chmod -R 777 on /artifacts with
OpenShift-friendly ownership and group-permission changes: create /artifacts,
chown it to root:0 (or the intended user:group), set the setgid bit so new files
inherit the group, and restrict permissions to something like 2775 (chmod
g+rwX,o-rwx) to allow group write without world-writable access; avoid granting
write permissions to "others" and remove the recursive 777 usage. Ensure the
Dockerfile command sequence updates ownership and permissions for /artifacts
accordingly.
In `@tests/integration/go.mod`:
- Line 32: The go.mod entry for the indirect dependency
"github.com/containerd/containerd v1.7.25" is vulnerable; update that module to
v1.7.29 (or later) by changing the version string in tests/integration/go.mod
from v1.7.25 to v1.7.29 and then refresh module metadata (e.g., run the
appropriate Go module update/cleanup such as go get
github.com/containerd/containerd@v1.7.29 and go mod tidy) so the go.sum and
transitive dependencies are updated accordingly.
- Line 52: The indirect dependency github.com/docker/docker is pinned to v28.5.2
which contains HIGH-severity CVEs; update that module to v29.3.1 or later
(recommend v29.4.3) by changing the version entry for github.com/docker/docker
to >= v29.3.1 in go.mod and then run the Go module update commands (e.g., go get
github.com/docker/docker@v29.4.3 and go mod tidy) to refresh go.sum and vendor
files so the tests/integration module uses the patched release.
In `@tests/integration/image-builders/operator/Makefile`:
- Around line 19-20: The push targets (push-bundles, push-related,
push-catalogs) currently separate the build and push with `;` so a failing build
can still allow a push; update each target's recipe to enable immediate failure
and prevent stale pushes by adding `set -e` at the start of the shell block
(inside the recipe before the for-loop) and replace the `build ...; \` /
`$(RUNTIME) push ...` sequence with `build ... && \` followed by the push so the
push only runs if the build succeeds; locate the three targets named
push-bundles, push-related, and push-catalogs and make these two changes in each
recipe.
- Around line 19-20: The Makefile assumes podman-specific flags but exposes
RUNTIME as generic; update the build/push invocations (where $(RUNTIME) is used)
to conditionally add podman-only --format flags only when RUNTIME is podman and
omit them for docker, and replace the invalid podman push format
"--format=docker" with a valid manifest format (e.g. "v2s2" or "oci") when using
podman; locate usages of $(RUNTIME) in the Makefile (the build and push commands
and the other occurrences mentioned) and implement a simple conditional around
those commands (or define separate PODMAN_ARGS/Docker_ARGS variables) so docker
build/push get no --format flag and podman build/push include the correct
--format values.
In `@tests/integration/image-builders/operator/related_image/run.sh`:
- Line 1: The script run.sh uses a shell shebang (#!/busybox/sh) but the image
gcr.io/distroless/static has no shell, so update the Dockerfile to use a base
image that includes a shell (for example busybox, alpine, or another standard
distro) or change the container startup to not rely on a shell; locate the
Dockerfile ENTRYPOINT/CMD that runs /run.sh and replace the FROM
gcr.io/distroless/static line with an appropriate shell-capable image (or
convert the script to a binary and use a distroless runtime image) so /run.sh
can execute.
In `@tests/integration/image-builders/release/artifacts/config/config.json`:
- Around line 12-13: The JSON is invalid because the "size" field uses a bare
SIZE token; replace the placeholder with a valid numeric sentinel (e.g., change
"size": SIZE to "size": 0) so the file remains valid JSON while still allowing
your existing textual sed replacement for SIZE/UPDATE_DIGEST in
create-release.sh to overwrite it at release time; leave the "digest":
"sha256:UPDATE_DIGEST" placeholder unchanged.
In `@tests/integration/image-builders/release/create-release.sh`:
- Around line 25-39: The script uses unquoted variable expansions (ART_DIGEST,
CONFIG_DIGEST, FILE_SIZE) which risks word-splitting; update all usages of these
variables (every cp, stat --printf, sed invocations, sha256sum | cut, and rm
-rf) to use double quotes around the expansions (e.g., "${ART_DIGEST}",
"${CONFIG_DIGEST}", "${FILE_SIZE}") so empty or whitespace-containing values are
safe and shellcheck SC2086 is resolved.
In `@tests/integration/image-builders/release/generate-release-signature.sh`:
- Around line 26-28: The script's hardcoded paths reference the old
images/release location causing the missing-file guard to trigger; change the
path logic to use SCRIPT_DIR (or set REPO_ROOT one level up) and point
INDEX_JSON and KEYS_DIR to the image-builders/release layout: declare
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" then set
INDEX_JSON="${SCRIPT_DIR}/release-payload/index.json" and
KEYS_DIR="${SCRIPT_DIR}/keys" (or alternatively set REPO_ROOT="$(cd "$(dirname
"${BASH_SOURCE[0]}")/.." && pwd)" and update INDEX_JSON and KEYS_DIR
accordingly) so the variables INDEX_JSON and KEYS_DIR resolve to the actual
image-builders/release files.
In `@tests/integration/image-builders/release/Makefile`:
- Around line 19-20: The push target in the Makefile hardcodes a developer-local
auth file; update the push recipe (target name: push) to use an overridable
variable (e.g. AUTHFILE) with a sensible default (for example
~/.docker/config.json) instead of /home/lzuccarelli/... so callers can run `make
push AUTHFILE=/path/to/auth.json` or rely on the default in CI; ensure the
podman push invocation uses the variable (AUTHFILE) for --authfile and document
the override in a comment or Makefile help.
- Around line 1-3: The Makefile declares .PHONY: all test build but defines no
recipe for the test target, so running make all silently skips tests; either add
a concrete test recipe (implement the integration test invocation under the test
target) or remove test from both .PHONY and the all prerequisite list. Also mark
container, digest, and push as phony (add them to the .PHONY list) because they
are recipe-only targets with no file outputs to avoid make treating them as
up-to-date files; update the .PHONY line to include container digest push (or
create a separate .PHONY line) and ensure the all target's prerequisites match
the actual recipes.
- Around line 9-10: The Makefile's clean target currently removes build/* but
the build target produces artifacts in bin/, leaving stale files; update the
clean target (the "clean" rule) to also remove bin/* or remove the entire bin
directory (e.g., rm -rf bin/* or rm -rf bin/) so that artifacts created by the
"build" target are cleaned up along with build/*.
In `@tests/integration/uid_entrypoint.sh`:
- Line 2: The script uses the bash-only redirection operator "&>" in the
conditional (the line containing "if ! whoami &> /dev/null; then"), which breaks
POSIX shells; change the redirection to a POSIX-compatible form by redirecting
stdout and stderr explicitly (e.g., replace "&>" with "> /dev/null 2>&1") so the
"whoami" call is silenced in POSIX sh.
---
Nitpick comments:
In `@tests/integration/image-builders/release/create-release.sh`:
- Around line 11-12: The script currently derives the image digest via podman
inspect (TMP_DIGEST/DIGEST) which can return the local manifest digest; update
the image push to capture the authoritative registry digest by adding
--digestfile to the push command in the Makefile's push target, and modify
create-release.sh to read the registry digest from that digestfile (instead of
using podman inspect TMP_DIGEST/DIGEST) so release manifests use the
registry-provided digest.
In `@tests/integration/image-builders/release/go.mod`:
- Around line 1-3: Replace the placeholder module declaration "module
example.com/m/v2" in the go.mod used by the image-builders/release test with a
project-scoped module path so tooling and IDEs resolve correctly; edit the
module line (the "module ..." entry in this go.mod) to a canonical
repository-specific path (for example a github.com/your-org/your-repo/... style
path that reflects this tests subtree) and run go mod tidy to ensure
dependencies and module metadata remain valid.
🪄 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: 9d5eadc7-c623-4a01-a36b-48b3bcde8580
⛔ Files ignored due to path filters (4)
tests/integration/go.sumis excluded by!**/*.sumtests/integration/image-builders/operator/bundles/bar/bar.svgis excluded by!**/*.svgtests/integration/image-builders/operator/bundles/baz/baz.svgis excluded by!**/*.svgtests/integration/image-builders/operator/bundles/foo/foo.svgis excluded by!**/*.svg
📒 Files selected for processing (79)
images/cli/Dockerfile.testtests/integration/d2m_invalid_from_test.gotests/integration/go.modtests/integration/image-builders/operator/Makefiletests/integration/image-builders/operator/bundles/bar/README.mdtests/integration/image-builders/operator/bundles/bar/bar-bundle-v0.1.0/Dockerfiletests/integration/image-builders/operator/bundles/bar/bar-bundle-v0.1.0/manifests/bar.csv.yamltests/integration/image-builders/operator/bundles/bar/bar-bundle-v0.1.0/manifests/bars.test.bar.crd.yamltests/integration/image-builders/operator/bundles/bar/bar-bundle-v0.1.0/metadata/annotations.yamltests/integration/image-builders/operator/bundles/bar/bar-bundle-v0.2.0/Dockerfiletests/integration/image-builders/operator/bundles/bar/bar-bundle-v0.2.0/manifests/bar.csv.yamltests/integration/image-builders/operator/bundles/bar/bar-bundle-v0.2.0/manifests/bars.test.bar.crd.yamltests/integration/image-builders/operator/bundles/bar/bar-bundle-v0.2.0/metadata/annotations.yamltests/integration/image-builders/operator/bundles/bar/bar-bundle-v1.0.0/Dockerfiletests/integration/image-builders/operator/bundles/bar/bar-bundle-v1.0.0/manifests/bar.csv.yamltests/integration/image-builders/operator/bundles/bar/bar-bundle-v1.0.0/manifests/bars.test.bar.crd.yamltests/integration/image-builders/operator/bundles/bar/bar-bundle-v1.0.0/metadata/annotations.yamltests/integration/image-builders/operator/bundles/baz/README.mdtests/integration/image-builders/operator/bundles/baz/baz-bundle-v1.0.0/Dockerfiletests/integration/image-builders/operator/bundles/baz/baz-bundle-v1.0.0/manifests/baz.csv.yamltests/integration/image-builders/operator/bundles/baz/baz-bundle-v1.0.0/manifests/bazs.test.baz.crd.yamltests/integration/image-builders/operator/bundles/baz/baz-bundle-v1.0.0/metadata/annotations.yamltests/integration/image-builders/operator/bundles/baz/baz-bundle-v1.0.1/Dockerfiletests/integration/image-builders/operator/bundles/baz/baz-bundle-v1.0.1/manifests/baz.csv.yamltests/integration/image-builders/operator/bundles/baz/baz-bundle-v1.0.1/manifests/bazs.test.baz.crd.yamltests/integration/image-builders/operator/bundles/baz/baz-bundle-v1.0.1/metadata/annotations.yamltests/integration/image-builders/operator/bundles/baz/baz-bundle-v1.1.0/Dockerfiletests/integration/image-builders/operator/bundles/baz/baz-bundle-v1.1.0/manifests/baz.csv.yamltests/integration/image-builders/operator/bundles/baz/baz-bundle-v1.1.0/manifests/bazs.test.baz.crd.yamltests/integration/image-builders/operator/bundles/baz/baz-bundle-v1.1.0/metadata/annotations.yamltests/integration/image-builders/operator/bundles/foo/README.mdtests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.1.0/Dockerfiletests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.1.0/manifests/foo.csv.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.1.0/manifests/foos.test.foo.crd.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.1.0/metadata/annotations.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.1.0/metadata/dependencies.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.1.1/Dockerfiletests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.1.1/manifests/foo.csv.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.1.1/manifests/foos.test.foo.crd.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.1.1/metadata/annotations.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.1.1/metadata/dependencies.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.2.0/Dockerfiletests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.2.0/manifests/foo.csv.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.2.0/manifests/foos.test.foo.crd.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.2.0/metadata/annotations.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.2.0/metadata/dependencies.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.0/Dockerfiletests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.0/manifests/foo.csv.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.0/manifests/foos.test.foo.crd.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.0/metadata/annotations.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.0/metadata/dependencies.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.1/Dockerfiletests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.1/manifests/foo.csv.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.1/manifests/foos.test.foo.crd.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.1/metadata/annotations.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.1/metadata/dependencies.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.2/Dockerfiletests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.2/manifests/foo.csv.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.2/manifests/foos.test.foo.crd.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.2/metadata/annotations.yamltests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.2/metadata/dependencies.yamltests/integration/image-builders/operator/cmd/build-catalogs/main.gotests/integration/image-builders/operator/related_image/Dockerfiletests/integration/image-builders/operator/related_image/run.shtests/integration/image-builders/release/Makefiletests/integration/image-builders/release/README.mdtests/integration/image-builders/release/artifacts/config/8366d414d18526e99f4781ed84a93b5b96ebe464d223e587cf7f705829b27a12tests/integration/image-builders/release/artifacts/config/config.jsontests/integration/image-builders/release/artifacts/release-manifests/image-referencestests/integration/image-builders/release/artifacts/release-manifests/image-references-basetests/integration/image-builders/release/artifacts/release-manifests/release-metadatatests/integration/image-builders/release/cmd/simple-release/main.gotests/integration/image-builders/release/containerfile-rhel9tests/integration/image-builders/release/create-release.shtests/integration/image-builders/release/generate-release-signature.shtests/integration/image-builders/release/go.modtests/integration/image-builders/release/release-payload/index.jsontests/integration/image-builders/release/release-payload/oci-layouttests/integration/uid_entrypoint.sh
✅ Files skipped from review due to trivial changes (50)
- tests/integration/image-builders/operator/bundles/bar/bar-bundle-v0.2.0/metadata/annotations.yaml
- tests/integration/image-builders/operator/bundles/baz/baz-bundle-v1.1.0/metadata/annotations.yaml
- tests/integration/image-builders/release/cmd/simple-release/main.go
- tests/integration/image-builders/operator/bundles/bar/README.md
- tests/integration/image-builders/release/release-payload/index.json
- tests/integration/image-builders/operator/bundles/baz/README.md
- tests/integration/image-builders/release/artifacts/config/8366d414d18526e99f4781ed84a93b5b96ebe464d223e587cf7f705829b27a12
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.1.0/metadata/annotations.yaml
- tests/integration/image-builders/operator/bundles/bar/bar-bundle-v0.2.0/Dockerfile
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.2/metadata/annotations.yaml
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.0/metadata/annotations.yaml
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.1.0/Dockerfile
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.2.0/metadata/dependencies.yaml
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.2/metadata/dependencies.yaml
- tests/integration/image-builders/operator/bundles/baz/baz-bundle-v1.0.0/Dockerfile
- tests/integration/image-builders/operator/bundles/baz/baz-bundle-v1.0.1/metadata/annotations.yaml
- tests/integration/image-builders/operator/bundles/bar/bar-bundle-v0.1.0/Dockerfile
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.1.1/metadata/dependencies.yaml
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.2.0/Dockerfile
- tests/integration/image-builders/release/release-payload/oci-layout
- tests/integration/image-builders/operator/bundles/baz/baz-bundle-v1.0.1/Dockerfile
- tests/integration/image-builders/operator/bundles/bar/bar-bundle-v0.2.0/manifests/bars.test.bar.crd.yaml
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.2.0/metadata/annotations.yaml
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.2/manifests/foos.test.foo.crd.yaml
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.0/metadata/dependencies.yaml
- tests/integration/image-builders/operator/bundles/baz/baz-bundle-v1.0.0/metadata/annotations.yaml
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.1.0/manifests/foos.test.foo.crd.yaml
- tests/integration/image-builders/operator/related_image/Dockerfile
- tests/integration/image-builders/release/artifacts/release-manifests/image-references-base
- tests/integration/image-builders/release/containerfile-rhel9
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.0/manifests/foos.test.foo.crd.yaml
- tests/integration/image-builders/operator/bundles/baz/baz-bundle-v1.0.0/manifests/baz.csv.yaml
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.1.1/manifests/foos.test.foo.crd.yaml
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.1/manifests/foos.test.foo.crd.yaml
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.1.1/metadata/annotations.yaml
- tests/integration/image-builders/operator/bundles/bar/bar-bundle-v0.1.0/metadata/annotations.yaml
- tests/integration/image-builders/operator/bundles/bar/bar-bundle-v1.0.0/Dockerfile
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.1.0/metadata/dependencies.yaml
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.2.0/manifests/foo.csv.yaml
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.0/manifests/foo.csv.yaml
- tests/integration/image-builders/operator/bundles/bar/bar-bundle-v0.1.0/manifests/bar.csv.yaml
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.2/manifests/foo.csv.yaml
- tests/integration/image-builders/operator/bundles/baz/baz-bundle-v1.0.1/manifests/baz.csv.yaml
- tests/integration/image-builders/operator/bundles/bar/bar-bundle-v0.2.0/manifests/bar.csv.yaml
- tests/integration/image-builders/operator/cmd/build-catalogs/main.go
- tests/integration/image-builders/operator/bundles/foo/foo-bundle-v0.3.1/metadata/annotations.yaml
- tests/integration/image-builders/operator/bundles/bar/bar-bundle-v1.0.0/metadata/annotations.yaml
- tests/integration/image-builders/operator/bundles/bar/bar-bundle-v0.1.0/manifests/bars.test.bar.crd.yaml
- tests/integration/image-builders/operator/bundles/foo/README.md
- tests/integration/image-builders/release/artifacts/release-manifests/image-references
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/d2m_invalid_from_test.go
adolfo-ab
force-pushed
the
add-integration-tests
branch
from
35fbcdb to
6bfbfd3
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
tests/integration/uid_entrypoint.sh (1)
2-2:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winUse POSIX-compatible stderr/stdout redirection.
The
&>operator is bash-specific and is undefined in POSIXsh(SC3020). Since the shebang is#!/bin/sh, this will silently misbehave or fail on shells likedashorash, which are common in minimal container images.🔧 Proposed fix
-if ! whoami &> /dev/null; then +if ! whoami >/dev/null 2>&1; then🤖 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/uid_entrypoint.sh` at line 2, The test uses a bash-only redirection operator in the conditional "if ! whoami &> /dev/null; then" which breaks under POSIX sh; change the redirection to a POSIX-compatible form by directing stdout to /dev/null and then redirecting stderr to stdout (i.e., replace the "&>" usage) so the "whoami" check works in dash/ash shells.tests/integration/go.mod (2)
32-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpgrade
containerdfromv1.7.25to a patched release.Line 32 is still pinned to a vulnerable version with multiple HIGH advisories in the provided scan output. Please bump to
v1.7.29or later and refresh module metadata.🔧 Proposed change
- github.com/containerd/containerd v1.7.25 // indirect + github.com/containerd/containerd v1.7.29 // indirect#!/bin/bash # Verify and update vulnerable containerd version in tests/integration module. set -euo pipefail cd tests/integration go get github.com/containerd/containerd@v1.7.29 go mod tidy go list -m github.com/containerd/containerd🤖 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/go.mod` at line 32, Update the pinned containerd module version from v1.7.25 to a patched release (>= v1.7.29) in the go.mod entry "github.com/containerd/containerd v1.7.25", then refresh module metadata by running a module update (e.g., run "go get github.com/containerd/containerd@v1.7.29" and "go mod tidy") in the tests/integration module so the go.sum and dependencies are updated to the patched release.
52-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpgrade
github.com/docker/dockerfromv28.5.2+incompatibleto a patched release.Line 52 still references a version flagged as HIGH severity in the provided scanner results. Please bump to
v29.3.1+incompatibleor newer and re-tidy.🔧 Proposed change
- github.com/docker/docker v28.5.2+incompatible // indirect + github.com/docker/docker v29.3.1+incompatible // indirect#!/bin/bash # Verify and update vulnerable docker/docker version in tests/integration module. set -euo pipefail cd tests/integration go get github.com/docker/docker@v29.3.1 go mod tidy go list -m github.com/docker/docker🤖 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/go.mod` at line 52, Update the indirect dependency version for github.com/docker/docker from v28.5.2+incompatible to v29.3.1+incompatible (or newer) in the module that declares it, then run module maintenance (go get github.com/docker/docker@v29.3.1 and go mod tidy) and verify with go list -m github.com/docker/docker to ensure the bumped version is recorded; target the occurrence of "github.com/docker/docker" in the go.mod entry and re-tidy the module after updating.
🤖 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/go.mod`:
- Line 32: Update the pinned containerd module version from v1.7.25 to a patched
release (>= v1.7.29) in the go.mod entry "github.com/containerd/containerd
v1.7.25", then refresh module metadata by running a module update (e.g., run "go
get github.com/containerd/containerd@v1.7.29" and "go mod tidy") in the
tests/integration module so the go.sum and dependencies are updated to the
patched release.
- Line 52: Update the indirect dependency version for github.com/docker/docker
from v28.5.2+incompatible to v29.3.1+incompatible (or newer) in the module that
declares it, then run module maintenance (go get
github.com/docker/docker@v29.3.1 and go mod tidy) and verify with go list -m
github.com/docker/docker to ensure the bumped version is recorded; target the
occurrence of "github.com/docker/docker" in the go.mod entry and re-tidy the
module after updating.
In `@tests/integration/uid_entrypoint.sh`:
- Line 2: The test uses a bash-only redirection operator in the conditional "if
! whoami &> /dev/null; then" which breaks under POSIX sh; change the redirection
to a POSIX-compatible form by directing stdout to /dev/null and then redirecting
stderr to stdout (i.e., replace the "&>" usage) so the "whoami" check works in
dash/ash shells.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 12a92598-510f-4945-89c0-07155ddca4fc
⛔ Files ignored due to path filters (1)
tests/integration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
Makefileimages/cli/Dockerfile.testtests/integration/go.modtests/integration/uid_entrypoint.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- images/cli/Dockerfile.test
adolfo-ab
force-pushed
the
add-integration-tests
branch
from
6bfbfd3 to
7dfdaa5
Compare
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
adolfo-ab
force-pushed
the
add-integration-tests
branch
from
7dfdaa5 to
abad517
Compare
adolfo-ab
force-pushed
the
add-integration-tests
branch
from
abad517 to
591639b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (5)
tests/integration/image-builders/release/Makefile (3)
9-11:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
cleantarget does not clean the artifacts produced bybuild.
buildwrites tobin/(Line 6), butcleanonly removesbuild/*(Line 10), so stale binaries remain.🧹 Proposed fix
clean: - rm -rf build/* + rm -rf build/* bin/ go clean ./...🤖 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/release/Makefile` around lines 9 - 11, The clean target currently removes build/* but misses artifacts produced by the build target (which writes to bin/); update the clean target (clean) to also remove the bin directory contents (e.g., rm -rf bin/* or rm -rf bin/) so stale binaries produced by the build target are deleted when running make clean; ensure you edit the Makefile's clean rule to include removal of bin alongside build.
1-3:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDefine
testor remove it fromall; also mark recipe-only targets as phony.Line 1 and Line 3 still reference
test, but there is notest:rule, somake allcan complete without running tests. Also,container,digest, andpushshould be phony to avoid file-name collisions.♻️ Proposed fix
-.PHONY: all test build clean +.PHONY: all build clean container digest push -all: clean test build +all: clean build + +# If test execution is required in this Makefile, add: +# test: +# go test ./...🤖 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/release/Makefile` around lines 1 - 3, The Makefile's "all" target references a missing "test" target and several recipe-only targets are not declared phony; either add a "test:" rule or remove "test" from the "all" prerequisites, and add the recipe-only targets to the .PHONY list. Specifically, update the .PHONY declaration to include test, container, digest, and push (and any other non-file targets) and then either implement a test target (e.g., "test:" with its commands) or delete "test" from the "all" dependency list to ensure "make all" actually invokes intended recipes and avoids filename collisions.
20-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a default for
AUTHFILEto prevent empty--authfileusage.Line 20 depends on
$(AUTHFILE), but no default is defined in this file. Add an overridable default to keep local and CI usage predictable.🔐 Proposed fix
+AUTHFILE ?= $(HOME)/.docker/config.json + .PHONY: all test build clean🤖 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/release/Makefile` at line 20, Add an overridable default for AUTHFILE so the podman push invocation doesn't get an empty --authfile value; declare AUTHFILE ?= /dev/null (or another sane default) near the top of the Makefile and leave the existing podman push line that references $(AUTHFILE) unchanged so CI and local runs have a predictable default while still allowing overrides.tests/integration/go.mod (2)
52-52:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH-severity docker/docker vulnerabilities remain unpatched—upgrade to v29.3.1+ required.
This issue was flagged in a previous review but hasn't been addressed. The indirect dependency
github.com/docker/docker v28.5.2contains two HIGH-severity CVEs published March 25, 2026:
Advisory CVE Summary GHSA-pxq6-2prw-chj9 / GO-2026-4883 CVE-2026-33997 Off-by-one error in plugin privilege validation GHSA-x744-4wpc-v9h2 / GO-2026-4887 CVE-2026-34040 AuthZ plugin bypass with oversized request bodies Upgrade to v29.3.1 or later (v29.4.3 is available as of May 2026) to resolve both issues.
🔧 Proposed fix
- github.com/docker/docker v28.5.2+incompatible // indirect + github.com/docker/docker v29.4.3+incompatible // indirectThen run:
cd tests/integration go get github.com/docker/docker@v29.4.3 go mod tidy🤖 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/go.mod` at line 52, Update the indirect dependency entry for github.com/docker/docker in the module declaration (the line containing "github.com/docker/docker v28.5.2+incompatible") to a patched release (v29.3.1 or later—recommend v29.4.3) and then run dependency resolution (e.g., in the tests/integration module run "go get github.com/docker/docker@v29.4.3" followed by "go mod tidy") so the high-severity CVEs are eliminated from the dependency graph.
32-32:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInconsistency:
containerdv1.7.25 is still vulnerable despite being marked "solved."The previous review flagged three HIGH-severity vulnerabilities in containerd v1.7.25, and you marked it as solved. However, the code still shows
v1.7.25, and static analysis continues to flag the same CVEs:
Advisory Issue Fixed in GHSA-265r-hfxg-fhmg / GO-2025-3528 Integer overflow in User ID handling v1.7.27+ GHSA-pwhc-rpq9-4c8w / GO-2025-4100 Local privilege escalation via CRI directory permissions v1.7.29+ GHSA-m6hq-p25p-ffr2 / GO-2025-4108 Host memory exhaustion through Attach goroutine leak v1.7.29+ Please upgrade to v1.7.29 or later to resolve all three vulnerabilities.
🔧 Proposed fix
- github.com/containerd/containerd v1.7.25 // indirect + github.com/containerd/containerd v1.7.29 // indirectThen run:
cd tests/integration go get github.com/containerd/containerd@v1.7.29 go mod tidy🤖 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/go.mod` at line 32, The go.mod entry for github.com/containerd/containerd is still pinned to v1.7.25 (symbol: "github.com/containerd/containerd" and version string "v1.7.25") which remains vulnerable; update that require line to v1.7.29 (or later) and then run the module bump commands (go get github.com/containerd/containerd@v1.7.29 and go mod tidy) so the dependency and go.sum are refreshed to resolve the listed CVEs.
🤖 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/go.mod`:
- Line 52: Update the indirect dependency entry for github.com/docker/docker in
the module declaration (the line containing "github.com/docker/docker
v28.5.2+incompatible") to a patched release (v29.3.1 or later—recommend v29.4.3)
and then run dependency resolution (e.g., in the tests/integration module run
"go get github.com/docker/docker@v29.4.3" followed by "go mod tidy") so the
high-severity CVEs are eliminated from the dependency graph.
- Line 32: The go.mod entry for github.com/containerd/containerd is still pinned
to v1.7.25 (symbol: "github.com/containerd/containerd" and version string
"v1.7.25") which remains vulnerable; update that require line to v1.7.29 (or
later) and then run the module bump commands (go get
github.com/containerd/containerd@v1.7.29 and go mod tidy) so the dependency and
go.sum are refreshed to resolve the listed CVEs.
In `@tests/integration/image-builders/release/Makefile`:
- Around line 9-11: The clean target currently removes build/* but misses
artifacts produced by the build target (which writes to bin/); update the clean
target (clean) to also remove the bin directory contents (e.g., rm -rf bin/* or
rm -rf bin/) so stale binaries produced by the build target are deleted when
running make clean; ensure you edit the Makefile's clean rule to include removal
of bin alongside build.
- Around line 1-3: The Makefile's "all" target references a missing "test"
target and several recipe-only targets are not declared phony; either add a
"test:" rule or remove "test" from the "all" prerequisites, and add the
recipe-only targets to the .PHONY list. Specifically, update the .PHONY
declaration to include test, container, digest, and push (and any other non-file
targets) and then either implement a test target (e.g., "test:" with its
commands) or delete "test" from the "all" dependency list to ensure "make all"
actually invokes intended recipes and avoids filename collisions.
- Line 20: Add an overridable default for AUTHFILE so the podman push invocation
doesn't get an empty --authfile value; declare AUTHFILE ?= /dev/null (or another
sane default) near the top of the Makefile and leave the existing podman push
line that references $(AUTHFILE) unchanged so CI and local runs have a
predictable default while still allowing overrides.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 485a1544-9052-464d-a7db-eaad4849a7f1
⛔ Files ignored due to path filters (1)
tests/integration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (17)
.gitignoreMakefileimages/cli/Dockerfile.testtests/integration/go.modtests/integration/image-builders/release/Makefiletests/integration/testdata/imagesetconfigs/exit_codes/isc-additional-image-error.yamltests/integration/testdata/imagesetconfigs/exit_codes/isc-generic-error.yamltests/integration/testdata/imagesetconfigs/exit_codes/isc-helm-error.yamltests/integration/testdata/imagesetconfigs/exit_codes/isc-operator-error.yamltests/integration/testdata/imagesetconfigs/exit_codes/isc-operator-helm-error.yamltests/integration/testdata/imagesetconfigs/exit_codes/isc-release-error.yamltests/integration/testdata/imagesetconfigs/happy_path/isc-happy-path.yamltests/integration/testdata/imagesetconfigs/operators/isc-operator-catalog-digest.yamltests/integration/testdata/imagesetconfigs/operators/isc-operator-invalid-version-range.yamltests/integration/testdata/imagesetconfigs/operators/isc-operator-pinned-version.yamltests/integration/testdata/imagesetconfigs/operators/isc-operator-version-range.yamltests/integration/uid_entrypoint.sh
✅ Files skipped from review due to trivial changes (9)
- tests/integration/testdata/imagesetconfigs/exit_codes/isc-operator-helm-error.yaml
- tests/integration/testdata/imagesetconfigs/exit_codes/isc-additional-image-error.yaml
- tests/integration/testdata/imagesetconfigs/exit_codes/isc-operator-error.yaml
- tests/integration/testdata/imagesetconfigs/happy_path/isc-happy-path.yaml
- tests/integration/testdata/imagesetconfigs/exit_codes/isc-generic-error.yaml
- tests/integration/testdata/imagesetconfigs/exit_codes/isc-helm-error.yaml
- tests/integration/testdata/imagesetconfigs/operators/isc-operator-invalid-version-range.yaml
- tests/integration/testdata/imagesetconfigs/operators/isc-operator-catalog-digest.yaml
- tests/integration/testdata/imagesetconfigs/operators/isc-operator-version-range.yaml
🚧 Files skipped from review as they are similar to previous changes (2)
- Makefile
- images/cli/Dockerfile.test
|
@adolfo-ab: 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. |
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
3 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
This PR moves the integration tests from https://github.com/oc-mirror-test/integration to oc-mirror itself.
The integration tests are a separate module within oc-mirror.
It removes the artifacts image and it also changes the registry that we use in the tests to an in-process registry.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Running the tests directly and from their container.
Expected Outcome
All the tests pass.
Summary by CodeRabbit