Skip to content

fake bump kmstokms test#712

Draft
gangwgr wants to merge 1 commit into
openshift:mainfrom
gangwgr:combine-all-operator-cases
Draft

fake bump kmstokms test#712
gangwgr wants to merge 1 commit into
openshift:mainfrom
gangwgr:combine-all-operator-cases

Conversation

@gangwgr

@gangwgr gangwgr commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

Release Notes

  • Tests

    • Refined KMS encryption end-to-end coverage by disabling two earlier scenarios and consolidating the remaining checks into shared helper flows, using isolated per-run namespaces and consistent primary/secondary provider configurations.
  • Chores

    • Updated Go version metadata outputs, including generating a new named Go version mapping file.
    • Adjusted module resolution by adding a dependency override to pin a specific library revision.

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 16, 2026
@openshift-ci

openshift-ci Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 85d88dec-c499-4734-8db4-a5f77f84977b

📥 Commits

Reviewing files that changed from the base of the PR and between 8d2ec38 and e7dd61f.

⛔ Files ignored due to path filters (6)
  • go.sum is excluded by !**/*.sum
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/key_controller.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/kms_preflight_controller.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/helpers.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/scenarios.go is excluded by !**/vendor/**, !vendor/**
  • vendor/modules.txt is excluded by !**/vendor/**, !vendor/**
📒 Files selected for processing (4)
  • _output/golang-versions
  • _output/named-golang-versions
  • go.mod
  • test/e2e-encryption-kms/encryption_kms.go
✅ Files skipped from review due to trivial changes (2)
  • _output/golang-versions
  • _output/named-golang-versions
🚧 Files skipped from review as they are similar to previous changes (2)
  • go.mod
  • test/e2e-encryption-kms/encryption_kms.go

Walkthrough

Library-go dependency is redirected to a fork in go.mod, two new KMS-to-KMS e2e test helpers are added to wire up library scenarios with Vault KMS provider setup, older KMS specs are commented out, and generated Go 1.25 version metadata files are created.

Changes

KMS-to-KMS encryption testing with library-go fork

Layer / File(s) Summary
library-go fork dependency override
go.mod
Added replace directive redirecting github.com/openshift/library-go to github.com/ardaguclu/library-go at a pinned commit.
KMS-to-KMS test helpers and spec updates
test/e2e-encryption-kms/encryption_kms.go
Adds testKMSToKMSMigration and testKMSToKMSOnOff helper functions, each creating a per-run namespace, setting up primary and secondary Vault KMS providers, and invoking the corresponding library test function with encrypted/non-encrypted assertions. Comments out TestKMSEncryptionOnOff and TestKMSEncryptionProvidersMigration specs while keeping TestKMSToKMSMigration and TestKMSToKMSOnOff active in the Ginkgo Describe block.
Generated Go 1.25 version metadata files
_output/golang-versions, _output/named-golang-versions
Creates output files recording Go version 1.25 for three entries, with named-golang-versions mapping to .ci-operator.yaml, go.mod, and Dockerfile.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • benluddy
  • ricardomaraschini

Important

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

❌ Failed checks (1 error, 2 warnings)

Check name Status Explanation Resolution
Container-Privileges ❌ Error PR introduces privileged containers: bindata/v3.11.0/openshift-apiserver/deploy.yaml contains two containers with privileged: true and runAsUser: 0; namespace enforces privileged pod security stand... Review justification for privileged containers (audit log access). Consider using allowPrivilegeEscalation: false and dropping unnecessary capabilities per container security best practices.
Title check ⚠️ Warning The title 'fake bump kmstokms test' is partially related to the changeset but is vague and misleading. It suggests a version bump when the actual changes involve updating Go version metadata, adding a module replace directive, and modifying test implementation for KMS-to-KMS migration tests. Use a more descriptive title that accurately reflects the main changes, such as 'Add KMS-to-KMS migration test implementation and update Go version to 1.25' or 'Implement KMS-to-KMS migration test cases with library-go redirect'.
Microshift Test Compatibility ⚠️ Warning Two new Ginkgo e2e tests use openshift-apiserver-operator APIs and control plane components unavailable on MicroShift without any skip mechanisms. Add [Skipped:MicroShift] label or guard tests with exutil.IsMicroShiftCluster() check since openshift-apiserver-operator and KMS encryption don't exist on MicroShift.
✅ Passed checks (12 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed All Ginkgo test titles (It, Describe) use static, deterministic names without dynamic values like timestamps, UUIDs, or random numbers. Dynamic namespace creation occurs only in test bodies, not te...
Test Structure And Quality ✅ Passed Code follows established patterns: Each It block tests one scenario via delegated library functions. Namespaces created with proper defer cleanup. Error assertions use require.NoError(t,err). Libra...
Single Node Openshift (Sno) Test Compatibility ✅ Passed Tests verify KMS encryption at the etcd/API layer without requiring multi-node infrastructure. SNO includes all necessary APIs (etcd, API Server, Routes). No SNO-incompatible operations detected.
Topology-Aware Scheduling Compatibility ✅ Passed The PR does not introduce deployment manifests, operator code, or controllers with topology-unfriendly scheduling constraints. Test file changes contain no scheduling logic, and the go.mod change i...
Ote Binary Stdout Contract ✅ Passed The test file contains no process-level stdout writes: TestMain only calls randomizeTestOrder and os.Exit; g.Describe registers tests without emitting to stdout; all fmt operations are inside test...
Ipv6 And Disconnected Network Test Compatibility ✅ Passed The new Ginkgo tests in encryption_kms.go contain no hardcoded IPv4 addresses, IPv4-only parsing, unsafe URL concatenation, or external connectivity calls; all operations are cluster-internal.
No-Weak-Crypto ✅ Passed No weak cryptography (MD5, SHA1, DES, RC4, 3DES, Blowfish, ECB), custom crypto implementations, or non-constant-time secret comparisons detected in the PR changes.
No-Sensitive-Data-In-Logs ✅ Passed No logging of sensitive data found. PR contains test code and build metadata with no exposed passwords, tokens, API keys, PII, or credentials in logs.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

@gangwgr

gangwgr commented Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-operator-encryption-kms

@openshift-ci

openshift-ci Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign ricardomaraschini for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gangwgr gangwgr force-pushed the combine-all-operator-cases branch from adb5d68 to b492d55 Compare June 16, 2026 12:24
@gangwgr

gangwgr commented Jun 16, 2026

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-operator-encryption-kms

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 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 `@go.mod`:
- Line 138: The replace directive for library-go at line 138 lacks an
explanatory comment that describes why the fork github.com/gangwgr/library-go is
needed, unlike other replace directives in the file. Add a comment above the
library-go replace directive that clarifies the purpose—whether this is a
temporary measure for testing a fix before merging upstream, or a permanent
change needed for ongoing compatibility. This will provide necessary context for
future maintainers about the rationale for using this fork instead of the
upstream library.

In `@test/e2e-encryption-kms/encryption_kms.go`:
- Around line 124-125: The namespace creation in
test/e2e-encryption-kms/encryption_kms.go at lines 124-125 (anchor location)
uses rand.IntN(4) which provides only 4 possible namespace names, causing test
collisions on reruns and shards. The same issue exists at lines 155-156 (sibling
location). Replace rand.IntN(4) with rand.IntN(100000) at both locations in the
fmt.Sprintf calls that generate the namespace names to significantly increase
entropy and prevent AlreadyExists flakes.
- Line 127: The deferred `Namespaces().Delete()` calls at lines 57, 94, 127, and
158 in the encryption_kms.go file all ignore the error return value, which can
cause leaked namespaces and cascade test failures. Modify each of the four
deferred cleanup calls to capture the error returned from `Delete()` and log or
handle it appropriately rather than ignoring it. Apply the same error handling
pattern consistently across all four locations where the deferred namespace
deletion occurs.
- Around line 140-141: Replace all instances of context.TODO() with the ctx
parameter that is accessible via closure capture from the enclosing test
function scope. In the operatorencryption.CreateAndStoreRouteOfLife call and any
other similar calls within the test setup closures, use the ctx parameter passed
to the test function instead of context.TODO() to ensure proper cancellation and
timeout propagation as required by Go security guidelines.
🪄 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: e3049b16-95dd-4649-ab05-2d0f341b7e5b

📥 Commits

Reviewing files that changed from the base of the PR and between 72b069a and b492d55.

⛔ Files ignored due to path filters (5)
  • go.sum is excluded by !**/*.sum
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/key_controller.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/helpers.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/scenarios.go is excluded by !**/vendor/**, !vendor/**
  • vendor/modules.txt is excluded by !**/vendor/**, !vendor/**
📒 Files selected for processing (4)
  • _output/golang-versions
  • _output/named-golang-versions
  • go.mod
  • test/e2e-encryption-kms/encryption_kms.go

Comment thread go.mod Outdated
// This replace is required for we use the OCP fork of Ginkgo.
replace github.com/onsi/ginkgo/v2 => github.com/openshift/onsi-ginkgo/v2 v2.6.1-0.20260303184444-1cc650aa0565

replace github.com/openshift/library-go => github.com/gangwgr/library-go v0.0.0-20260616121325-7a109a33686c

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

# First, let's examine the go.mod file to understand the full context
head -150 go.mod | tail -20

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 1060


🏁 Script executed:

# Check the full go.mod file for context around the replace directive
cat -n go.mod | grep -A 5 -B 5 "gangwgr/library-go"

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 417


🏁 Script executed:

# Check if there's any git history or comments explaining this change
git log --oneline -p go.mod 2>/dev/null | head -200 || echo "Git history not available"

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 7122


🌐 Web query:

gangwgr/library-go fork security openshift

💡 Result:

There is no evidence of a security-related fork of the OpenShift library-go repository maintained by user 'gangwgr' [1][2][3]. The user 'gangwgr' is an active contributor to the official openshift/library-go repository, as evidenced by numerous merged pull requests and commits to the main codebase [1][3][4][5]. The references to 'gangwgr' in various OpenShift operators (such as cluster-kube-apiserver-operator and cluster-openshift-apiserver-operator) are strictly for bumping dependencies to the latest versions of the official library-go repository, not to a personal or security fork [6][7]. In OpenShift development, it is common practice to use temporary 'replace' directives in go.mod files to point to personal forks when testing specific fixes or new features before they are merged into the official repository [8][9][10]. While other contributors have used such temporary forks for development purposes in the past [8][9], there is no indication that 'gangwgr' maintains a security-specific fork of library-go [1][3]. The library-go repository itself remains the canonical source for OpenShift library helpers [11][2].

Citations:


🌐 Web query:

openshift/library-go known CVEs osv.dev

💡 Result:

The openshift/library-go repository does not have a single, static list of known CVEs published directly on OSV.dev. Because library-go is a collection of helper libraries used heavily within the OpenShift ecosystem, security vulnerabilities affecting it are typically tracked through specific OpenShift Bugzilla issues (OCPBUGS), Red Hat Security Advisories (RHSA), or by updating its dependencies [1][2][3]. To check for vulnerabilities related to this package using OSV.dev, you can use the OSV API or web interface by querying the package using its Package URL (PURL): pkg:golang/github.com/openshift/library-go [4][5]. Key points regarding vulnerability management for this repository include: 1. Dependency Updates: Many CVEs associated with library-go are actually vulnerabilities in its upstream dependencies (such as logrus or various Kubernetes libraries) [1][2]. Updates to library-go are frequently issued to pull in patched versions of these dependencies [1][2][6]. 2. Tracking Mechanism: Vulnerabilities are often documented in Red Hat's internal issue tracker (referenced via OCPBUGS IDs in public GitHub pull requests) or via official Red Hat Security Advisories [1][2][3]. 3. Querying OSV.dev: You can query vulnerabilities programmatically by sending a POST request to the OSV API with the package details [4][5]: curl -X POST -d '{"package": {"name": "github.com/openshift/library-go", "ecosystem": "Go"}}' "https://api.osv.dev/v1/query" Historical examples of vulnerabilities linked to this repository or its sub-packages include CVE-2025-65637 (a DoS vulnerability fixed by updating a dependency) [1][2], CVE-2023-0229 [7], and CVE-2019-10213 [8]. Always verify the current status of your specific version by checking the Go module dependencies or consulting official OpenShift release notes [9][10].

Citations:


Add explanatory comment to the library-go replace directive.

Line 138 lacks context explaining why the fork is needed (unlike the Ginkgo replace on line 135-136, which includes a comment). Add a comment clarifying the purpose—whether this is temporary for testing before merging a fix, or permanent for ongoing compatibility.

🤖 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 `@go.mod` at line 138, The replace directive for library-go at line 138 lacks
an explanatory comment that describes why the fork github.com/gangwgr/library-go
is needed, unlike other replace directives in the file. Add a comment above the
library-go replace directive that clarifies the purpose—whether this is a
temporary measure for testing a fix before merging upstream, or a permanent
change needed for ongoing compatibility. This will provide necessary context for
future maintainers about the rationale for using this fork instead of the
upstream library.

Source: Coding guidelines

Comment on lines +124 to +125
ns := fmt.Sprintf("test-kms-to-kms-migration-%d", rand.IntN(4))
_, err := cs.KubeClient.CoreV1().Namespaces().Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}}, metav1.CreateOptions{})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Increase namespace entropy to avoid test collisions.

Line 124 and Line 155 use rand.IntN(4), which gives only 4 possible namespace names; this is prone to AlreadyExists flakes across reruns/shards.

💡 Suggested fix
- ns := fmt.Sprintf("test-kms-to-kms-migration-%d", rand.IntN(4))
+ ns := fmt.Sprintf("test-kms-to-kms-migration-%d", rand.Uint64())

- ns := fmt.Sprintf("test-kms-to-kms-onoff-%d", rand.IntN(4))
+ ns := fmt.Sprintf("test-kms-to-kms-onoff-%d", rand.Uint64())

Also applies to: 155-156

🤖 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 `@test/e2e-encryption-kms/encryption_kms.go` around lines 124 - 125, The
namespace creation in test/e2e-encryption-kms/encryption_kms.go at lines 124-125
(anchor location) uses rand.IntN(4) which provides only 4 possible namespace
names, causing test collisions on reruns and shards. The same issue exists at
lines 155-156 (sibling location). Replace rand.IntN(4) with rand.IntN(100000) at
both locations in the fmt.Sprintf calls that generate the namespace names to
significantly increase entropy and prevent AlreadyExists flakes.

ns := fmt.Sprintf("test-kms-to-kms-migration-%d", rand.IntN(4))
_, err := cs.KubeClient.CoreV1().Namespaces().Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}}, metav1.CreateOptions{})
require.NoError(t, err)
defer cs.KubeClient.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, locate the file and check its content around the specified lines
wc -l test/e2e-encryption-kms/encryption_kms.go

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 136


🏁 Script executed:

# Read the file around lines 127 and 158 to verify the issue
sed -n '120,165p' test/e2e-encryption-kms/encryption_kms.go

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 3107


🏁 Script executed:

# Check the imports in the file to see if require is available
head -30 test/e2e-encryption-kms/encryption_kms.go

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 1260


🏁 Script executed:

# Check for other Delete() calls that might have similar issues
grep -n "\.Delete(" test/e2e-encryption-kms/encryption_kms.go

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 436


🏁 Script executed:

# Also search for similar patterns with deferred operations that might ignore errors
grep -n "defer.*\\.Delete\\|defer.*\\.Delete(" test/e2e-encryption-kms/encryption_kms.go

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 436


Fix ignored errors in deferred namespace cleanup across all instances.

Lines 57, 94, 127, and 158 all ignore errors from Namespaces().Delete() in deferred calls. Cleanup errors can cause leaked namespaces and cascade into subsequent e2e test failures. Per Go coding guidelines, error returns must never be ignored.

🛠️ Suggested fix
- defer cs.KubeClient.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{})
+ defer func() {
+ 	err := cs.KubeClient.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{})
+ 	require.NoError(t, err)
+ }()

Apply this pattern to all four occurrences (lines 57, 94, 127, and 158).

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
defer cs.KubeClient.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{})
defer func() {
err := cs.KubeClient.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{})
require.NoError(t, 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 `@test/e2e-encryption-kms/encryption_kms.go` at line 127, The deferred
`Namespaces().Delete()` calls at lines 57, 94, 127, and 158 in the
encryption_kms.go file all ignore the error return value, which can cause leaked
namespaces and cascade test failures. Modify each of the four deferred cleanup
calls to capture the error returned from `Delete()` and log or handle it
appropriately rather than ignoring it. Apply the same error handling pattern
consistently across all four locations where the deferred namespace deletion
occurs.

Source: Coding guidelines

Comment on lines +140 to +141
return operatorencryption.CreateAndStoreRouteOfLife(context.TODO(), t, operatorencryption.GetClients(t), ns)
},

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

fd -t f encryption_kms.go

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 132


🏁 Script executed:

wc -l test/e2e-encryption-kms/encryption_kms.go

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 136


🏁 Script executed:

sed -n '130,180p' test/e2e-encryption-kms/encryption_kms.go

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 3466


🏁 Script executed:

sed -n '100,145p' test/e2e-encryption-kms/encryption_kms.go

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 3141


🏁 Script executed:

sed -n '155,180p' test/e2e-encryption-kms/encryption_kms.go

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 1880


🏁 Script executed:

sed -n '70,130p' test/e2e-encryption-kms/encryption_kms.go | head -70

Repository: openshift/cluster-openshift-apiserver-operator

Length of output: 3774


Replace context.TODO() with the ctx parameter passed to the enclosing test functions.

Lines 140 and 171 use context.TODO() within closures where ctx is accessible via closure capture from the surrounding function scope. This prevents proper cancellation and timeout propagation as required by Go security guidelines.

Suggested fixes

Line 140:

- return operatorencryption.CreateAndStoreRouteOfLife(context.TODO(), t, operatorencryption.GetClients(t), ns)
+ return operatorencryption.CreateAndStoreRouteOfLife(ctx, t, operatorencryption.GetClients(t), ns)

Line 171:

- return operatorencryption.CreateAndStoreRouteOfLife(context.TODO(), t, operatorencryption.GetClients(t), ns)
+ return operatorencryption.CreateAndStoreRouteOfLife(ctx, t, operatorencryption.GetClients(t), ns)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
return operatorencryption.CreateAndStoreRouteOfLife(context.TODO(), t, operatorencryption.GetClients(t), ns)
},
return operatorencryption.CreateAndStoreRouteOfLife(ctx, t, operatorencryption.GetClients(t), ns)
},
🤖 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 `@test/e2e-encryption-kms/encryption_kms.go` around lines 140 - 141, Replace
all instances of context.TODO() with the ctx parameter that is accessible via
closure capture from the enclosing test function scope. In the
operatorencryption.CreateAndStoreRouteOfLife call and any other similar calls
within the test setup closures, use the ctx parameter passed to the test
function instead of context.TODO() to ensure proper cancellation and timeout
propagation as required by Go security guidelines.

Source: Coding guidelines

@openshift-ci

openshift-ci Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor

@gangwgr: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-aws-operator-encryption-kms b492d55 link false /test e2e-aws-operator-encryption-kms

Full PR test history. Your PR dashboard.

Details

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 kubernetes-sigs/prow repository. I understand the commands that are listed here.

@gangwgr

gangwgr commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

/test e2e-aws-operator-encryption-kms

@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 17, 2026
@gangwgr gangwgr closed this Jun 17, 2026
@gangwgr gangwgr reopened this Jun 17, 2026
@gangwgr gangwgr force-pushed the combine-all-operator-cases branch from b492d55 to 8d2ec38 Compare June 17, 2026 10:38
@openshift-ci openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 17, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
go.mod (1)

138-138: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Missing explanatory comment and supply chain justification for library-go fork.

Line 138 introduces a replace directive for github.com/openshift/library-go pointing to a personal fork (github.com/ardaguclu/library-go), but provides no explanatory comment. This mirrors a previous finding and additionally violates the coding guideline requirement to "justify need" for new dependencies.

Per coding guidelines, you must:

  1. Add a comment clarifying whether this is temporary (e.g., testing a fix pre-merge) or permanent, consistent with the Ginkgo replace pattern at line 135–136.
  2. Justify the fork: explain why the personal fork is necessary instead of upstream github.com/openshift/library-go.
  3. Document the pre-release status: the version v0.0.0-20260617092928-ed7dbb06ff70 is a development snapshot (v0.0.0), not a release; confirm this is appropriate for your use case.
💡 Suggested comment format
-replace github.com/openshift/library-go => github.com/ardaguclu/library-go v0.0.0-20260617092928-ed7dbb06ff70
+// TODO: Temporary fork to test KMS-to-KMS migration support; merge upstream before release.
+replace github.com/openshift/library-go => github.com/ardaguclu/library-go v0.0.0-20260617092928-ed7dbb06ff70

(Adjust the comment to match the actual justification.)

🤖 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 `@go.mod` at line 138, The replace directive for
github.com/openshift/library-go pointing to the personal fork
github.com/ardaguclu/library-go lacks required documentation. Add a comment
above this replace directive explaining whether this is a temporary fork (e.g.,
testing a fix pre-merge) or permanent, justify why the personal fork is
necessary instead of using the upstream version, and document that the version
v0.0.0-20260617092928-ed7dbb06ff70 is a development snapshot to confirm this is
appropriate for the use case. Follow the same documentation pattern used for the
Ginkgo replace directive to maintain consistency.

Source: Coding guidelines

🤖 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 `@go.mod`:
- Line 138: The replace directive for github.com/openshift/library-go pointing
to the personal fork github.com/ardaguclu/library-go lacks required
documentation. Add a comment above this replace directive explaining whether
this is a temporary fork (e.g., testing a fix pre-merge) or permanent, justify
why the personal fork is necessary instead of using the upstream version, and
document that the version v0.0.0-20260617092928-ed7dbb06ff70 is a development
snapshot to confirm this is appropriate for the use case. Follow the same
documentation pattern used for the Ginkgo replace directive to maintain
consistency.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 3ee819ee-8a06-4cf8-8210-387d0960b090

📥 Commits

Reviewing files that changed from the base of the PR and between b492d55 and 8d2ec38.

⛔ Files ignored due to path filters (5)
  • go.sum is excluded by !**/*.sum
  • vendor/github.com/openshift/library-go/pkg/operator/encryption/controllers/key_controller.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/helpers.go is excluded by !**/vendor/**, !vendor/**
  • vendor/github.com/openshift/library-go/test/library/encryption/scenarios.go is excluded by !**/vendor/**, !vendor/**
  • vendor/modules.txt is excluded by !**/vendor/**, !vendor/**
📒 Files selected for processing (4)
  • _output/golang-versions
  • _output/named-golang-versions
  • go.mod
  • test/e2e-encryption-kms/encryption_kms.go
✅ Files skipped from review due to trivial changes (2)
  • _output/named-golang-versions
  • _output/golang-versions
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/e2e-encryption-kms/encryption_kms.go

@gangwgr

gangwgr commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

/testwith openshift/cluster-openshift-apiserver-operator/main/e2e-aws-operator-encryption-kms https://github.com/openshift/cluster-openshift-kube-apiserver-operator/pull/2203 openshift/cluster-authentication-operator#917

@openshift-ci

openshift-ci Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

@gangwgr, testwith: Error processing request. ERROR:

could not determine job runs: couldn't get PR from GitHub: openshift/cluster-openshift-kube-apiserver-operator#2203: status code 404 not one of [200], body: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/pulls/pulls#get-a-pull-request","status":"404"}

@gangwgr

gangwgr commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

/testwith openshift/cluster-openshift-apiserver-operator/main/e2e-aws-operator-encryption-kms https://github.com/openshift/cluster-openshift-kube-apiserver-operator/pull/2203 openshift/cluster-authentication-operator#917

@openshift-ci

openshift-ci Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

@gangwgr, testwith: Error processing request. ERROR:

could not determine job runs: couldn't get PR from GitHub: openshift/cluster-openshift-kube-apiserver-operator#2203: status code 404 not one of [200], body: {"message":"Not Found","documentation_url":"https://docs.github.com/rest/pulls/pulls#get-a-pull-request","status":"404"}

@gangwgr

gangwgr commented Jun 17, 2026

Copy link
Copy Markdown
Contributor Author

/testwith openshift/cluster-openshift-apiserver-operator/main/e2e-aws-operator-encryption-kms openshift/cluster-kube-apiserver-operator#2203 openshift/cluster-authentication-operator#917

@gangwgr gangwgr force-pushed the combine-all-operator-cases branch from 8d2ec38 to e7dd61f Compare June 18, 2026 06:08
@gangwgr

gangwgr commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

/testwith openshift/cluster-openshift-apiserver-operator/main/e2e-aws-operator-encryption-kms openshift/cluster-kube-apiserver-operator#2203 openshift/cluster-authentication-operator#917

@openshift-ci openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2026
@openshift-ci

openshift-ci Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

PR needs rebase.

Details

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 kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant