Conversation
|
@adolfo-ab: This pull request references CLID-479 which is a valid jira issue. 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. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 36 minutes and 8 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
WalkthroughAdds a new tests-extension module: build tooling and Dockerfile, a Go CLI test-extension binary with e2e suite registration and hooks, extensive e2e utilities for registry/operator orchestration, bindata support, and many testdata YAML fixtures; also updates Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer/CI
participant Builder as Go Builder (Docker)
participant Runtime as Runtime Image
participant Cluster as OpenShift Cluster
participant TestExt as tests-extension binary
participant Registry as Image Registry
Dev->>Builder: run multi-stage build (vendor, bindata, go build, package)
Builder->>Runtime: copy gzipped binary artifact into runtime image
Dev->>Cluster: deploy runtime image / run test jobs
Cluster->>TestExt: execute tests-extension (Cobra entrypoint)
TestExt->>Cluster: apply templates, install operators, create registries, poll readiness
TestExt->>Registry: query tags/signatures, validate registry APIs
TestExt->>Cluster: run e2e suites and report results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings, 1 inconclusive)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
adolfo-ab
force-pushed
the
ote-migration
branch
from
1cb2698 to
edb3977
Compare
|
[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 |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (8)
tests-extension/test/e2e/testdata/workloads/config-83875.yaml (1)
7-7: Adjust indentation for consistency.Line 7 uses 7 spaces before the list item dash, while the standard pattern in other configuration files uses 8 spaces (maintaining 2-space increments). This inconsistency could affect YAML parsing and makes the codebase harder to maintain.
🔧 Proposed fix
packages: - - name: recipe + - name: recipe🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests-extension/test/e2e/testdata/workloads/config-83875.yaml` at line 7, Adjust the indentation of the list item "- name: recipe" in config-83875.yaml to use 8 spaces before the dash (consistent 2-space increments) instead of the current 7 spaces so it matches the rest of the workload configs and avoids YAML parsing/formatting inconsistencies; update the entry in the same list block that contains "name: recipe" to align with other items.tests-extension/test/e2e/testdata/workloads/config-79408.yaml (1)
14-15: Prefer a pinned digest/version over:latestfor stable e2e behavior.
Usingregistry.redhat.io/ubi8/ubi:latestcan make this fixture nondeterministic as upstream tags move. Consider pinning to an immutable digest (or a fixed version tag).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests-extension/test/e2e/testdata/workloads/config-79408.yaml` around lines 14 - 15, The test fixture uses an image name with a mutable tag ("additionalImages" -> name: "registry.redhat.io/ubi8/ubi:latest"); replace the mutable :latest with an immutable identifier by pinning to a specific version tag or digest (for example a fixed semver tag or an image@sha256:<digest>) to make e2e runs deterministic—update the value of the name entry under additionalImages accordingly.tests-extension/test/e2e/testdata/workloads/config-88132-target-repo-tag.yaml (1)
5-13: Consider replacing:latestsource tags with fixed versions/digests.
For an e2e fixture, mutable tags can make outcomes drift over time and reduce reproducibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests-extension/test/e2e/testdata/workloads/config-88132-target-repo-tag.yaml` around lines 5 - 13, The fixture uses mutable :latest tags for the image entries (docker://registry.redhat.io/ubi9/ubi-minimal:latest, registry.access.redhat.com/ubi8/nginx-120:latest, docker://quay.io/fedora/fedora:latest) which makes e2e results non-deterministic; update each of these names in the YAML to use a fixed semver tag or an immutable image digest (sha256) instead (for example replace :latest with a specific release tag or `@sha256`:<digest>) so tests reference exact image content—locate the three name keys and substitute deterministic tags/digests for ubi-minimal, nginx-120, and fedora entries.tests-extension/test/e2e/testdata/workloads/config-72972.yaml (1)
5-5: Use an immutable reference for the UBI image to reduce test drift.
registry.redhat.io/ubi8/ubi:latestcan change over time; pinning a digest keeps this workload fixture reproducible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests-extension/test/e2e/testdata/workloads/config-72972.yaml` at line 5, The manifest currently uses a mutable tag "registry.redhat.io/ubi8/ubi:latest" in the workload fixture; replace that tag with an immutable digest reference (registry.redhat.io/ubi8/ubi@sha256:<actual-digest>) so the test fixture is reproducible. Locate the "name" field containing "registry.redhat.io/ubi8/ubi:latest" in the config-72972.yaml test workload and update it to the exact image digest retrieved from the Red Hat registry (or your approved fixture digest), ensuring you include the full "@sha256:..." digest string.tests-extension/test/e2e/testdata/workloads/config-74660.yaml (1)
5-5: Fix typo in comment."ooperator" should be "operator".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests-extension/test/e2e/testdata/workloads/config-74660.yaml` at line 5, Fix the typo in the inline comment for the 'graph' key: change the word "ooperator" to "operator" in the comment following graph: true so it reads "Required for the OSUS operator"; update the comment text near the 'graph' key to the corrected spelling.tests-extension/test/e2e/testdata/workloads/config-74650-minor-v1.yaml (1)
9-9: Fix typo in comment."ooperator" should be "operator".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests-extension/test/e2e/testdata/workloads/config-74650-minor-v1.yaml` at line 9, Fix the typo in the inline comment for the "graph" setting: change "Required for the OSUS ooperator" to "Required for the OSUS operator" so the comment on the graph: true line reads correctly.tests-extension/test/e2e/testdata/workloads/config-74650-patch-v2.yaml (1)
9-9: Minor typo in comment.The comment has a typo: "ooperator" should be "operator".
✏️ Proposed fix
- graph: true # Required for the OSUS ooperator + graph: true # Required for the OSUS operator🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests-extension/test/e2e/testdata/workloads/config-74650-patch-v2.yaml` at line 9, Fix the typo in the inline comment next to the graph key: replace "Required for the OSUS ooperator" with "Required for the OSUS operator" in the same line containing "graph: true" so the comment reads correctly.tests-extension/Makefile (1)
20-24: Consider adding conventionalallandtestphony targetsTo improve compatibility with common Makefile tooling and repo checks, add minimal
allandtesttargets (even iftestdelegates or is a no-op placeholder for now).Suggested change
+.PHONY: all +all: build + +.PHONY: test +test: + `@echo` "No tests target defined for tests-extension yet" + .PHONY: help help: `@echo` "Available targets:" `@echo` " build - Build extension binary" `@echo` " clean - Remove binaries and bindata" + `@echo` " test - Run tests (placeholder)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests-extension/Makefile` around lines 20 - 24, Add conventional phony targets "all" and "test" to the Makefile alongside the existing "help", "build", and "clean" targets: declare them in the .PHONY list and implement "all" to delegate to the existing "build" target and "test" to either run the repo's test command (e.g., "go test ./...") or be a no-op placeholder that prints a message so common tools and checks find these targets; update the .PHONY line and add short recipe bodies for the "all" and "test" targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile.tests-extension`:
- Around line 21-30: The final image stage runs as root by default; add a
non-root runtime user and ensure the copied test extension binary is
owned/accessible by that user: create a non-root user/group (e.g., uid 1001) in
the final stage, chown the copied file from /usr/bin/ (the artifact copied from
the test-extension-builder stage) to that user, and set USER to that non-root
user so the image no longer runs as root at runtime; also ensure any directories
the process needs are writable by that user.
- Line 11: Replace the non-reproducible install of go-bindata that uses the
`@latest` tag by pinning it to a specific released version: change the GOFLAGS= go
install github.com/go-bindata/go-bindata/v3/go-bindata@latest invocation to use
a concrete semver tag (for example `@v3.x.y`) so builds are reproducible and
immune to upstream drift; ensure the chosen version is a known-good release and
update any related lockfiles or build metadata accordingly.
In `@tests-extension/cmd/main.go`:
- Around line 27-29: The e2e framework flags registered by
util.InitStandardFlags() are never parsed because flag.CommandLine isn't bound
to the Cobra root and framework.AfterReadingAllFlags(&framework.TestContext)
runs before Cobra parses flags; fix by calling
root.PersistentFlags().AddGoFlagSet(flag.CommandLine) after creating the root
command and moving the framework.AfterReadingAllFlags(&framework.TestContext)
invocation into a root.PersistentPreRun (or PersistentPreRunE) hook so it runs
after Cobra has parsed CLI flags; keep util.InitStandardFlags() where it is but
ensure the new AddGoFlagSet and PersistentPreRun changes are applied (also
update the same pattern for the other occurrence around lines 75-83).
In `@tests-extension/go.mod`:
- Around line 6-15: The go.mod in tests-extension declares direct requires for
github.com/onsi/ginkgo/v2 v2.28.1 and k8s.io/* (apimachinery/component-base
v0.34.1 and kubernetes v1.34.1) but has replacements to OpenShift forks
(github.com/openshift/onsi-ginkgo and github.com/openshift/kubernetes) that are
on different, older versions; update tests-extension/go.mod so the required
versions match the replaced fork versions (or remove/adjust the replacements to
match the direct requires) to keep a consistent fork stack: either change the
require lines for github.com/onsi/ginkgo/v2 and k8s.io/*/kubernetes to the exact
versions used by the openshift replacements, or update the replace entries
(openshift/onsi-ginkgo, openshift/kubernetes) to point at tags/commits that
correspond to v2.28.1 and v1.34.1; also scan other go.mod files in the repo for
occurrences of github.com/onsi/ginkgo/v2, github.com/openshift/onsi-ginkgo,
k8s.io/(apimachinery|component-base|kubernetes) and align them all to the same
set of versions so tests-extension/cmd/main.go imports resolve consistently.
In `@tests-extension/test/e2e/bindata.mk`:
- Around line 5-8: The make rule for $(GO_BINDATA) currently installs go-bindata
with `@latest` which makes generated bindata.go non-reproducible; change the go
install line in the $(GO_BINDATA) target to pin a fixed semver (replace `@latest`
with a specific tag like `@vX.Y.Z`), update any related documentation/variables if
needed, and ensure the pinned version is committed so the verify-bindata target
continues to match exact generated output.
In `@tests-extension/test/e2e/testdata/workloads/config-72708.yaml`:
- Line 5: Replace the mutable image tag "registry.redhat.io/ubi8/ubi:latest" in
the fixture with a pinned digest so the test is reproducible; locate the
occurrence of that image string in config-72708.yaml and change the tag form to
the image@sha256:<digest> form (obtain the correct sha256 by pulling the image
or querying the registry / manifest API) and commit the updated YAML.
In `@tests-extension/test/e2e/testdata/workloads/config-72913.yaml`:
- Around line 10-15: The fixture uses mutable image tags: the catalog field
value "registry.redhat.io/redhat/redhat-operator-index:v4.15" and the
additionalImages entry "registry.redhat.io/rhel8/support-tools:latest"; replace
both with immutable digests (pin to sha256 digests) so the test’s mirrored
payload is stable—update the catalog value and the additionalImages entries to
their corresponding image@sha256:<digest> forms and commit the updated config.
In `@tests-extension/test/e2e/testdata/workloads/config-72949-2.yaml`:
- Around line 6-8: The catalog field uses a mutable tag; update the YAML so the
catalog key in this fixture is pinned to an immutable digest (replace catalog:
registry.redhat.io/redhat/redhat-marketplace-index:v4.15 with a digest form like
registry.redhat.io/redhat/redhat-marketplace-index@sha256:<actual-digest>) so
the mirror test is stable; leave targetCatalog and targetTag as-is but ensure
the chosen sha256 digest corresponds to the v4.15 content you intend to lock.
In `@tests-extension/test/e2e/testdata/workloads/config-73783.yaml`:
- Line 6: The OCI image digest in the image reference string
"quay.io/coreos/etcd:v3.5.4@sha256:a67fb152d4c53223e96e818420c37f11d05c2d92cf62c05ca5604066c37295e9ab"
is 66 hex chars instead of the required 64; remove the trailing two characters
("ab") so the digest is exactly 64 hexadecimal characters (ending with
"...c37295e9a") to produce a valid sha256 digest and prevent image-pull
failures.
In `@tests-extension/test/e2e/testdata/workloads/customsub.yaml`:
- Around line 6-8: The YAML mapping contains a duplicated key "kind:
Subscription" (appearing twice) which can cause parser-dependent behavior;
remove the redundant "kind: Subscription" entry so the mapping only declares
kind once (keep the single "kind: Subscription" line alongside the existing
"apiVersion: operators.coreos.com/v1alpha1"), ensuring the file has one unique
kind declaration.
In `@tests-extension/test/e2e/testdata/workloads/delete-config-72972.yaml`:
- Line 5: Replace the floating image tag used in the test fixture by pinning it
to a digest: locate the image string "registry.redhat.io/ubi8/ubi:latest" in the
test data and change it to a digest-pinned reference (for example
"registry.redhat.io/ubi8/ubi@sha256:<actual-digest>") so the e2e workload
becomes deterministic and matches the other digest-pinned image in this file.
In `@tests-extension/test/e2e/testdata/workloads/sa-79215.yaml`:
- Around line 6-30: The manifest binds an over-privileged wildcard ClusterRole
("test-79215-installer-admin-clusterrole") to the ServiceAccount "test-79215";
narrow the permissions by replacing the wildcards in apiGroups/resources/verbs
with only the actual API groups, resource names and verbs this workload needs,
and if the workload does not require cluster-wide access convert the
ClusterRole/ClusterRoleBinding to a namespaced Role and RoleBinding (keep the
subject ServiceAccount name "test-79215" and namespace "ns-79215" and update
roleRef/kind/name accordingly) so the binding is limited to the ns-79215
namespace.
In `@tests-extension/test/e2e/util.go`:
- Line 444: The fmt.Printf call printing the variable file uses the wrong format
specifier (%f) for a string; change the format specifier in the println call
that references file (the fmt.Printf(...) line) to a string specifier such as %s
or %q so the output is rendered correctly (e.g., replace %f with %s).
- Around line 598-600: The code currently checks the wrong response variable—use
tagsResp instead of resp—so replace the condition that tests resp.StatusCode
with one that tests tagsResp.StatusCode and update the error message to
reference tagsURL (or include tagsResp.Status) so a failed tags request is
correctly detected; look for the block that uses resp, tagsResp and tagsURL to
make this change.
- Around line 810-812: Remove the leftover debug sleep and print: delete the
fmt.Println("Debug......") and time.Sleep(20 * time.Minute) calls in
tests-extension/test/e2e/util.go and invoke e2e.Failf("All pods related to
deployment are not running") immediately (or replace the sleep with a proper
retry/backoff routine if intended); ensure the failing path uses e2e.Failf as
shown so CI won't hang for 20 minutes.
- Around line 856-858: The conditional that checks for presence of both
signature-configmap.yaml and signature-configmap.json in
signatureConfigmapOutput is incorrect: it currently uses if !yaml && json which
only fails when YAML is missing and JSON present; change it to fail when either
file is missing by checking if !(strings.Contains(signatureConfigmapOutput,
"signature-configmap.yaml") && strings.Contains(signatureConfigmapOutput,
"signature-configmap.json")) or equivalently if !strings.Contains(... "yaml") ||
!strings.Contains(... "json") and then call e2e.Failf as before; update the
condition around signatureConfigmapOutput in tests-extension/test/e2e/util.go
accordingly.
---
Nitpick comments:
In `@tests-extension/Makefile`:
- Around line 20-24: Add conventional phony targets "all" and "test" to the
Makefile alongside the existing "help", "build", and "clean" targets: declare
them in the .PHONY list and implement "all" to delegate to the existing "build"
target and "test" to either run the repo's test command (e.g., "go test ./...")
or be a no-op placeholder that prints a message so common tools and checks find
these targets; update the .PHONY line and add short recipe bodies for the "all"
and "test" targets.
In `@tests-extension/test/e2e/testdata/workloads/config-72972.yaml`:
- Line 5: The manifest currently uses a mutable tag
"registry.redhat.io/ubi8/ubi:latest" in the workload fixture; replace that tag
with an immutable digest reference
(registry.redhat.io/ubi8/ubi@sha256:<actual-digest>) so the test fixture is
reproducible. Locate the "name" field containing
"registry.redhat.io/ubi8/ubi:latest" in the config-72972.yaml test workload and
update it to the exact image digest retrieved from the Red Hat registry (or your
approved fixture digest), ensuring you include the full "@sha256:..." digest
string.
In `@tests-extension/test/e2e/testdata/workloads/config-74650-minor-v1.yaml`:
- Line 9: Fix the typo in the inline comment for the "graph" setting: change
"Required for the OSUS ooperator" to "Required for the OSUS operator" so the
comment on the graph: true line reads correctly.
In `@tests-extension/test/e2e/testdata/workloads/config-74650-patch-v2.yaml`:
- Line 9: Fix the typo in the inline comment next to the graph key: replace
"Required for the OSUS ooperator" with "Required for the OSUS operator" in the
same line containing "graph: true" so the comment reads correctly.
In `@tests-extension/test/e2e/testdata/workloads/config-74660.yaml`:
- Line 5: Fix the typo in the inline comment for the 'graph' key: change the
word "ooperator" to "operator" in the comment following graph: true so it reads
"Required for the OSUS operator"; update the comment text near the 'graph' key
to the corrected spelling.
In `@tests-extension/test/e2e/testdata/workloads/config-79408.yaml`:
- Around line 14-15: The test fixture uses an image name with a mutable tag
("additionalImages" -> name: "registry.redhat.io/ubi8/ubi:latest"); replace the
mutable :latest with an immutable identifier by pinning to a specific version
tag or digest (for example a fixed semver tag or an image@sha256:<digest>) to
make e2e runs deterministic—update the value of the name entry under
additionalImages accordingly.
In `@tests-extension/test/e2e/testdata/workloads/config-83875.yaml`:
- Line 7: Adjust the indentation of the list item "- name: recipe" in
config-83875.yaml to use 8 spaces before the dash (consistent 2-space
increments) instead of the current 7 spaces so it matches the rest of the
workload configs and avoids YAML parsing/formatting inconsistencies; update the
entry in the same list block that contains "name: recipe" to align with other
items.
In
`@tests-extension/test/e2e/testdata/workloads/config-88132-target-repo-tag.yaml`:
- Around line 5-13: The fixture uses mutable :latest tags for the image entries
(docker://registry.redhat.io/ubi9/ubi-minimal:latest,
registry.access.redhat.com/ubi8/nginx-120:latest,
docker://quay.io/fedora/fedora:latest) which makes e2e results
non-deterministic; update each of these names in the YAML to use a fixed semver
tag or an immutable image digest (sha256) instead (for example replace :latest
with a specific release tag or `@sha256`:<digest>) so tests reference exact image
content—locate the three name keys and substitute deterministic tags/digests for
ubi-minimal, nginx-120, and fedora entries.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ae92345-2672-4965-8a32-845a5769060a
⛔ Files ignored due to path filters (1)
tests-extension/go.sumis excluded by!**/*.sum
📒 Files selected for processing (78)
.gitignoreDockerfile.tests-extensiontests-extension/Makefiletests-extension/cmd/main.gotests-extension/go.modtests-extension/test/e2e/bindata.mktests-extension/test/e2e/oc_mirror_v2.gotests-extension/test/e2e/testdata/bindata.gotests-extension/test/e2e/testdata/fixtures.gotests-extension/test/e2e/testdata/workloads/case83849/config-83849.yamltests-extension/test/e2e/testdata/workloads/case83849/test-mirror-helm-0.3.0.tgztests-extension/test/e2e/testdata/workloads/case83864/test-mirror-helm-err.tgztests-extension/test/e2e/testdata/workloads/case84007/config-84007.yamltests-extension/test/e2e/testdata/workloads/case84007/operators.lsttests-extension/test/e2e/testdata/workloads/ceFile-79215.yamltests-extension/test/e2e/testdata/workloads/config-72708.yamltests-extension/test/e2e/testdata/workloads/config-72913.yamltests-extension/test/e2e/testdata/workloads/config-72917.yamltests-extension/test/e2e/testdata/workloads/config-72920.yamltests-extension/test/e2e/testdata/workloads/config-72938-1.yamltests-extension/test/e2e/testdata/workloads/config-72938.yamltests-extension/test/e2e/testdata/workloads/config-72942.yamltests-extension/test/e2e/testdata/workloads/config-72948.yamltests-extension/test/e2e/testdata/workloads/config-72949-1.yamltests-extension/test/e2e/testdata/workloads/config-72949-2.yamltests-extension/test/e2e/testdata/workloads/config-72971.yamltests-extension/test/e2e/testdata/workloads/config-72972.yamltests-extension/test/e2e/testdata/workloads/config-72973.yamltests-extension/test/e2e/testdata/workloads/config-72982.yamltests-extension/test/e2e/testdata/workloads/config-72983.yamltests-extension/test/e2e/testdata/workloads/config-73124.yamltests-extension/test/e2e/testdata/workloads/config-73359.yamltests-extension/test/e2e/testdata/workloads/config-73377.yamltests-extension/test/e2e/testdata/workloads/config-73452.yamltests-extension/test/e2e/testdata/workloads/config-73783.yamltests-extension/test/e2e/testdata/workloads/config-73791.yamltests-extension/test/e2e/testdata/workloads/config-74649.yamltests-extension/test/e2e/testdata/workloads/config-74650-minor-v1.yamltests-extension/test/e2e/testdata/workloads/config-74650-minor-v2.yamltests-extension/test/e2e/testdata/workloads/config-74650-patch-v1.yamltests-extension/test/e2e/testdata/workloads/config-74650-patch-v2.yamltests-extension/test/e2e/testdata/workloads/config-74660.yamltests-extension/test/e2e/testdata/workloads/config-75422-delete.yamltests-extension/test/e2e/testdata/workloads/config-75422.yamltests-extension/test/e2e/testdata/workloads/config-75425.yamltests-extension/test/e2e/testdata/workloads/config-75437.yamltests-extension/test/e2e/testdata/workloads/config-75438.yamltests-extension/test/e2e/testdata/workloads/config-76469.yamltests-extension/test/e2e/testdata/workloads/config-76489.yamltests-extension/test/e2e/testdata/workloads/config-76596.yamltests-extension/test/e2e/testdata/workloads/config-76597-delete.yamltests-extension/test/e2e/testdata/workloads/config-76597.yamltests-extension/test/e2e/testdata/workloads/config-77060.yamltests-extension/test/e2e/testdata/workloads/config-79215.yamltests-extension/test/e2e/testdata/workloads/config-79217-1.yamltests-extension/test/e2e/testdata/workloads/config-79217.yamltests-extension/test/e2e/testdata/workloads/config-79408.yamltests-extension/test/e2e/testdata/workloads/config-79452-v1.yamltests-extension/test/e2e/testdata/workloads/config-79452-v2.yamltests-extension/test/e2e/testdata/workloads/config-83582.yamltests-extension/test/e2e/testdata/workloads/config-83875.yamltests-extension/test/e2e/testdata/workloads/config-86309.yamltests-extension/test/e2e/testdata/workloads/config-87992.yamltests-extension/test/e2e/testdata/workloads/config-88132-digest.yamltests-extension/test/e2e/testdata/workloads/config-88132-invalid.yamltests-extension/test/e2e/testdata/workloads/config-88132-target-repo-tag.yamltests-extension/test/e2e/testdata/workloads/config-88132-target-repo.yamltests-extension/test/e2e/testdata/workloads/config-88132-target-tag.yamltests-extension/test/e2e/testdata/workloads/customsub.yamltests-extension/test/e2e/testdata/workloads/delete-config-72708.yamltests-extension/test/e2e/testdata/workloads/delete-config-72972.yamltests-extension/test/e2e/testdata/workloads/delete-config-77061.yamltests-extension/test/e2e/testdata/workloads/delete-config-79217.yamltests-extension/test/e2e/testdata/workloads/delete-config-79452.yamltests-extension/test/e2e/testdata/workloads/ibmcustomsub.yamltests-extension/test/e2e/testdata/workloads/operatorgroup.yamltests-extension/test/e2e/testdata/workloads/sa-79215.yamltests-extension/test/e2e/util.go
|
/retest |
aguidirh
left a comment
aguidirh
left a comment
There was a problem hiding this comment.
Thanks for the PR @adolfo-ab
I left some comments based on what I read from the OTE migration doc.
Could you please see why the sanity is failing and if does code rabbit suggestions make sense?
|
@adolfo-ab: This pull request references CLID-479 which is a valid jira issue. 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. |
|
@adolfo-ab: This pull request references CLID-479 which is a valid jira issue. 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Dockerfile.tests-extension (1)
8-18: Reduce drift by delegating build steps totests-extension/Makefile.Lines 8-15 duplicate vendoring/bindata/build logic already maintained in
tests-extension/Makefile. Calling the Make target here keeps flags and generation flow in one place.Proposed refactor
RUN cd tests-extension && \ - GOTOOLCHAIN=auto go mod vendor && \ - GOPATH=$(go env GOPATH) && \ - GOFLAGS= go install github.com/go-bindata/go-bindata/v3/go-bindata@latest && \ - $GOPATH/bin/go-bindata -nocompress \ - -pkg testdata -o test/e2e/testdata/bindata.go -prefix "testdata" test/e2e/testdata/... && \ - gofmt -s -w test/e2e/testdata/bindata.go && \ - CGO_ENABLED=0 GO_COMPLIANCE_POLICY="exempt_all" GOTOOLCHAIN=auto go build -mod=vendor -o bin/oc-mirror-tests-ext ./cmd && \ + $(MAKE) build && \ cd bin && \ tar -czvf oc-mirror-test-extension.tar.gz oc-mirror-tests-ext && \ rm -f oc-mirror-tests-ext🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Dockerfile.tests-extension` around lines 8 - 18, The Dockerfile.tests-extension RUN block duplicates vendoring, bindata generation, formatting and build steps that are already implemented in tests-extension/Makefile; replace the multi-step RUN sequence with a single make invocation (e.g., call the appropriate Makefile target such as the tests-extension Makefile's vendor/bindata/build or a combined target) so that Docker delegates GOTOOLCHAIN, go mod vendor, go-bindata generation, gofmt, and the CGO_DISABLED build to the Makefile; ensure the Docker RUN uses `cd tests-extension && make <target>` (the target name present in the Makefile) and then archives the produced binary in bin/oc-mirror-tests-ext as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests-extension/Makefile`:
- Around line 20-24: Add missing phony targets to the Makefile: declare "all"
and "test" in the .PHONY list and implement simple targets named all and test
consistent with other project Makefiles (e.g., make all should depend on build;
make test should run the project's test command or a placeholder invocation used
elsewhere). Update the .PHONY line to include all and test and add the
corresponding target stubs (all: build and test: <test-command>) so the file
matches the project's Makefile conventions.
---
Nitpick comments:
In `@Dockerfile.tests-extension`:
- Around line 8-18: The Dockerfile.tests-extension RUN block duplicates
vendoring, bindata generation, formatting and build steps that are already
implemented in tests-extension/Makefile; replace the multi-step RUN sequence
with a single make invocation (e.g., call the appropriate Makefile target such
as the tests-extension Makefile's vendor/bindata/build or a combined target) so
that Docker delegates GOTOOLCHAIN, go mod vendor, go-bindata generation, gofmt,
and the CGO_DISABLED build to the Makefile; ensure the Docker RUN uses `cd
tests-extension && make <target>` (the target name present in the Makefile) and
then archives the produced binary in bin/oc-mirror-tests-ext as before.
🪄 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: Pro Plus
Run ID: 00021127-eafd-477a-bde0-685fbdd6cc16
📒 Files selected for processing (2)
Dockerfile.tests-extensiontests-extension/Makefile
|
@adolfo-ab: This pull request references CLID-479 which is a valid jira issue. 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. |
adolfo-ab
force-pushed
the
ote-migration
branch
from
7d9bac2 to
14f93cb
Compare
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn openshift/origin#31006 |
|
@adolfo-ab: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/7c82a9e0-37f1-11f1-8f47-552584c66ffc-0 |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn openshift/origin#31006 |
|
@adolfo-ab: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/3268a3e0-38c4-11f1-8097-e51627f73738-0 |
adolfo-ab
force-pushed
the
ote-migration
branch
from
3a37c5f to
1aa5934
Compare
|
@adolfo-ab: This pull request references CLID-479 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 epic to target either version "5.0." or "openshift-5.0.", but it targets "openshift-4.22" instead. 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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile.tests-extension`:
- Around line 30-34: The LABEL instruction for com.redhat.component is malformed
due to a newline inside the quoted value; fix the LABEL block so the
com.redhat.component value is a single continuous quoted string (e.g.,
com.redhat.component="...-extension") and preserve the line-continuation
backslashes for other labels if needed; ensure no stray newline or unmatched
quote remains in the LABEL statement so the Docker build no longer fails.
🪄 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: Pro Plus
Run ID: baee69a5-1025-4af8-879e-4c2f97230579
📒 Files selected for processing (5)
Dockerfile.tests-extensionimages/cli/Dockerfile.citests-extension/cmd/main.gotests-extension/test/e2e/oc_mirror_v2.gotests-extension/test/e2e/testdata/bindata.go
🚧 Files skipped from review as they are similar to previous changes (1)
- tests-extension/cmd/main.go
adolfo-ab
force-pushed
the
ote-migration
branch
from
1aa5934 to
f63e5dd
Compare
|
@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. |
|
/payload-job-with-prs periodic-ci-openshift-release-main-ci-5.0-e2e-aws-ovn openshift/origin#31006 |
|
@adolfo-ab: trigger 1 job(s) for the /payload-(with-prs|job|aggregate|job-with-prs|aggregate-with-prs) command
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/dc7638b0-38d9-11f1-8041-6bf03a0e77d4-0 |
3 participants
Description
Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.
Github / Jira issue:
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
New Features
Tests
Chores