bump library-go funcs#935
Open
gangwgr wants to merge 1 commit into
Open
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
Contributor
|
Skipping CI for Draft Pull Request. |
WalkthroughThe PR adds a go.mod replace directive for ChangesLibrary-go replacement and KMS helper migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 4❌ Failed checks (3 warnings, 1 inconclusive)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
/test e2e-aws-operator-encryption-kms |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e-encryption-kms/encryption_kms.go (1)
15-23: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRegister the new KMS scenarios in the Ginkgo suite.
testKMSToKMSMigrationandtestKMSToKMSOnOffare never referenced here, so the new coverage never runs. golangci-lint is already flagging Line 92 and Line 116 as unused.Possible fix
g.It("TestKMSEncryptionProvidersMigration [OCPFeatureGate:KMSEncryption][Serial][Timeout:120m]", func(ctx context.Context) { testKMSEncryptionProvidersMigration(ctx, g.GinkgoTB()) }) + +g.It("TestKMSToKMSMigration [OCPFeatureGate:KMSEncryption][Serial][Timeout:120m]", func(ctx context.Context) { + testKMSToKMSMigration(ctx, g.GinkgoTB()) +}) + +g.It("TestKMSToKMSOnOff [OCPFeatureGate:KMSEncryption][Serial][Timeout:120m]", func(ctx context.Context) { + testKMSToKMSOnOff(ctx, g.GinkgoTB()) +})🤖 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 15 - 23, The new KMS test helpers are not being invoked from the Ginkgo suite, so their coverage is missing and they remain unused. Update the suite registration in the Describe block to include both testKMSToKMSMigration and testKMSToKMSOnOff alongside the existing KMS scenarios, using their existing Ginkgo.It wrappers so they execute under the same KMSEncryption suite context and stop golangci-lint from reporting them as unused.Source: Linters/SAST tools
🤖 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 146: The go.mod replace is too broad because it rewires every
github.com/openshift/library-go import to the fork. Remove the repo-wide replace
and scope the change to the specific KMS helper dependency instead, or
vendor/upstream the needed patch so only the affected symbols are redirected.
Use the existing go.mod replace entry as the place to adjust the dependency
mapping.
In `@test/e2e-encryption-kms/encryption_kms.go`:
- Around line 47-49: The token creation callbacks are ignoring the test’s
cancellation and timeout by calling context.TODO(), which can let resource setup
outlive the test. Update all four CreateResourceFunc closures in
encryption_kms.go to pass through the surrounding ctx instead of using
context.TODO(), and keep the existing library.CreateAndStoreTokenOfLife call
structure unchanged.
---
Outside diff comments:
In `@test/e2e-encryption-kms/encryption_kms.go`:
- Around line 15-23: The new KMS test helpers are not being invoked from the
Ginkgo suite, so their coverage is missing and they remain unused. Update the
suite registration in the Describe block to include both testKMSToKMSMigration
and testKMSToKMSOnOff alongside the existing KMS scenarios, using their existing
Ginkgo.It wrappers so they execute under the same KMSEncryption suite context
and stop golangci-lint from reporting them as unused.
🪄 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: 61c2a0c4-4d7b-45aa-be8e-39f5409ad596
⛔ Files ignored due to path filters (7)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/library-go/test/library/encryption/assertion_auth.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/assertion_oas.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/helpers_auth.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/helpers_oas.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (2)
go.modtest/e2e-encryption-kms/encryption_kms.go
|
|
||
| replace github.com/onsi/ginkgo/v2 => github.com/openshift/onsi-ginkgo/v2 v2.6.1-0.20251001123353-fd5b1fb35db1 | ||
|
|
||
| replace github.com/openshift/library-go => github.com/gangwgr/library-go v0.0.0-20260629113541-3c7eab1cd26e |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo-wide library-go imports:"
rg -n --type=go '"github.com/openshift/library-go' .
echo
echo "go.mod replace directives:"
sed -n '140,150p' go.mod
echo
echo "Dependency override metadata (if present):"
fd 'deps\.diff$|go\.sum$' . -x sh -c 'echo "== $1 =="; sed -n "1,120p" "$1"' sh {}Repository: openshift/cluster-authentication-operator
Length of output: 50397
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "go.mod replace block:"
sed -n '130,155p' go.mod
echo
echo "Any fork references in repo metadata:"
rg -n --hidden --glob '!vendor/**' 'gangwgr|replace github.com/openshift/library-go' go.mod vendor/modules.txt deps.diff . || true
echo
echo "library-go entries in vendor/modules.txt (if present):"
if [ -f vendor/modules.txt ]; then
rg -n 'github.com/openshift/library-go|github.com/gangwgr/library-go' vendor/modules.txt
fiRepository: openshift/cluster-authentication-operator
Length of output: 7318
Avoid the repo-wide library-go replace
This rewires every github.com/openshift/library-go import in the module, not just the KMS helpers. If this is carrying a local patch, keep it as an explicit vendored patch or upstream the helper instead of pinning the whole repo to the fork.
🤖 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 146, The go.mod replace is too broad because it rewires every
github.com/openshift/library-go import to the fork. Remove the repo-wide replace
and scope the change to the specific KMS helper dependency instead, or
vendor/upstream the needed patch so only the affected symbols are redirected.
Use the existing go.mod replace entry as the place to adjust the dependency
mapping.
Sources: Coding guidelines, Path instructions
Comment on lines
47
to
49
| CreateResourceFunc: func(t testing.TB, _ library.ClientSet, namespace string) runtime.Object { | ||
| return operatorencryption.CreateAndStoreTokenOfLife(context.TODO(), t, operatorencryption.GetClients(t)) | ||
| return library.CreateAndStoreTokenOfLife(context.TODO(), t, library.GetClients(t)) | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file outline ==\n'
ast-grep outline test/e2e-encryption-kms/encryption_kms.go --view expanded || true
printf '\n== relevant lines ==\n'
nl -ba test/e2e-encryption-kms/encryption_kms.go | sed -n '1,220p'
printf '\n== search for CreateAndStoreTokenOfLife ==\n'
rg -n "CreateAndStoreTokenOfLife|CreateResourceFunc|context.TODO\(\)" test/e2e-encryption-kms test -g '*.go'
printf '\n== inspect function definition ==\n'
rg -n "func .*CreateAndStoreTokenOfLife|CreateAndStoreTokenOfLife\\(" -g '*.go'Repository: openshift/cluster-authentication-operator
Length of output: 665
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== encryption_kms.go lines 30-150 ==\n'
sed -n '30,150p' test/e2e-encryption-kms/encryption_kms.go | cat -n
printf '\n== CreateAndStoreTokenOfLife definition and usages ==\n'
rg -n "func .*CreateAndStoreTokenOfLife|CreateAndStoreTokenOfLife\\(" -g '*.go' .
printf '\n== related helper signatures near library package ==\n'
rg -n "type .*ClientSet|CreateAndStoreTokenOfLife|GetClients\\(" test pkg -g '*.go'Repository: openshift/cluster-authentication-operator
Length of output: 9902
Pass the test context through the token-creation callbacks.
context.TODO() drops the outer ctx’s timeout/cancellation, so these paths can outlive the test. Use ctx in all four CreateResourceFunc closures.
🤖 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 47 - 49, The token
creation callbacks are ignoring the test’s cancellation and timeout by calling
context.TODO(), which can let resource setup outlive the test. Update all four
CreateResourceFunc closures in encryption_kms.go to pass through the surrounding
ctx instead of using context.TODO(), and keep the existing
library.CreateAndStoreTokenOfLife call structure unchanged.
Sources: Coding guidelines, Path instructions
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
ardaguclu
reviewed
Jun 29, 2026
| ResourceFunc: func(t testing.TB, _ string) runtime.Object { return operatorencryption.TokenOfLife(t) }, | ||
| AssertResourceEncryptedFunc: library.AssertTokenOfLifeEncrypted, | ||
| AssertResourceNotEncryptedFunc: library.AssertTokenOfLifeNotEncrypted, | ||
| ResourceFunc: func(_ testing.TB, _ string) runtime.Object { return library.TokenOfLife() }, |
Member
There was a problem hiding this comment.
Suggested change
| ResourceFunc: func(_ testing.TB, _ string) runtime.Object { return library.TokenOfLife() }, | |
| ResourceFunc: library.TokenOfLife()``` |
ardaguclu
reviewed
Jun 29, 2026
| ResourceFunc: func(t testing.TB, _ string) runtime.Object { return operatorencryption.TokenOfLife(t) }, | ||
| AssertResourceEncryptedFunc: library.AssertTokenOfLifeEncrypted, | ||
| AssertResourceNotEncryptedFunc: library.AssertTokenOfLifeNotEncrypted, | ||
| ResourceFunc: func(_ testing.TB, _ string) runtime.Object { return library.TokenOfLife() }, |
Member
There was a problem hiding this comment.
Suggested change
| ResourceFunc: func(_ testing.TB, _ string) runtime.Object { return library.TokenOfLife() }, | |
| ResourceFunc: library.TokenOfLife(), |
gangwgr
force-pushed
the
bump-library-go-funcs
branch
from
8909e07 to
fe4549a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e-encryption-kms/encryption_kms.go`:
- Around line 91-137: The new KMS-to-KMS helper scenarios in
testKMSToKMSMigration and testKMSToKMSOnOff are currently dead code because
nothing calls them from the test entrypoint. Update the existing encryption test
registration/dispatch path to invoke these helpers alongside the other scenarios
so the new coverage actually runs, keeping the wiring consistent with the
surrounding test functions in this package.
🪄 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: cd708cd8-5d0c-4386-852b-9ed454d78573
⛔ Files ignored due to path filters (7)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/library-go/test/library/encryption/assertion_auth.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/assertion_oas.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/helpers.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/helpers_auth.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/helpers_oas.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (2)
go.modtest/e2e-encryption-kms/encryption_kms.go
✅ Files skipped from review due to trivial changes (1)
- go.mod
Comment on lines
+91
to
+137
| // testKMSToKMSMigration tests KMS-to-KMS migration (primary → secondary → primary → identity). | ||
| func testKMSToKMSMigration(ctx context.Context, t testing.TB) { | ||
| library.TestKMSToKMSMigration(ctx, t, library.KMSToKMSMigrationScenario{ | ||
| BasicScenario: library.BasicScenario{ | ||
| Namespace: "openshift-config-managed", | ||
| LabelSelector: "encryption.apiserver.operator.openshift.io/component" + "=" + "openshift-oauth-apiserver", | ||
| EncryptionConfigSecretName: "encryption-config-openshift-oauth-apiserver", | ||
| EncryptionConfigSecretNamespace: "openshift-config-managed", | ||
| OperatorNamespace: "openshift-authentication-operator", | ||
| TargetGRs: library.AuthTargetGRs, | ||
| AssertFunc: library.AssertTokens, | ||
| }, | ||
| CreateResourceFunc: func(t testing.TB, _ library.ClientSet, namespace string) runtime.Object { | ||
| return library.CreateAndStoreTokenOfLife(context.TODO(), t, library.GetClients(t)) | ||
| }, | ||
| AssertResourceEncryptedFunc: library.AssertTokenOfLifeEncrypted, | ||
| AssertResourceNotEncryptedFunc: library.AssertTokenOfLifeNotEncrypted, | ||
| ResourceFunc: library.TokenOfLife, | ||
| ResourceName: "TokenOfLife", | ||
| PrimaryProvider: librarykms.DefaultVaultEncryptionProvider(ctx, t), | ||
| SecondaryProvider: librarykms.SecondaryVaultEncryptionProvider(ctx, t), | ||
| }) | ||
| } | ||
|
|
||
| // testKMSToKMSOnOff tests KMS on/off cycle with two distinct KMS providers. | ||
| func testKMSToKMSOnOff(ctx context.Context, t testing.TB) { | ||
| library.TestKMSToKMSOnOff(ctx, t, library.KMSToKMSMigrationScenario{ | ||
| BasicScenario: library.BasicScenario{ | ||
| Namespace: "openshift-config-managed", | ||
| LabelSelector: "encryption.apiserver.operator.openshift.io/component" + "=" + "openshift-oauth-apiserver", | ||
| EncryptionConfigSecretName: "encryption-config-openshift-oauth-apiserver", | ||
| EncryptionConfigSecretNamespace: "openshift-config-managed", | ||
| OperatorNamespace: "openshift-authentication-operator", | ||
| TargetGRs: library.AuthTargetGRs, | ||
| AssertFunc: library.AssertTokens, | ||
| }, | ||
| CreateResourceFunc: func(t testing.TB, _ library.ClientSet, namespace string) runtime.Object { | ||
| return library.CreateAndStoreTokenOfLife(context.TODO(), t, library.GetClients(t)) | ||
| }, | ||
| AssertResourceEncryptedFunc: library.AssertTokenOfLifeEncrypted, | ||
| AssertResourceNotEncryptedFunc: library.AssertTokenOfLifeNotEncrypted, | ||
| ResourceFunc: library.TokenOfLife, | ||
| ResourceName: "TokenOfLife", | ||
| PrimaryProvider: librarykms.DefaultVaultEncryptionProvider(ctx, t), | ||
| SecondaryProvider: librarykms.SecondaryVaultEncryptionProvider(ctx, t), | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Wire these new scenarios into the test entrypoint.
Line 92 and Line 116 add helper functions that nothing invokes, so the new KMS-to-KMS migration/on-off paths never execute. Please register them alongside the existing encryption scenarios; otherwise this PR adds dead test code instead of coverage.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 92-92: func testKMSToKMSMigration is unused
(unused)
[error] 116-116: func testKMSToKMSOnOff is unused
(unused)
🤖 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 91 - 137, The new
KMS-to-KMS helper scenarios in testKMSToKMSMigration and testKMSToKMSOnOff are
currently dead code because nothing calls them from the test entrypoint. Update
the existing encryption test registration/dispatch path to invoke these helpers
alongside the other scenarios so the new coverage actually runs, keeping the
wiring consistent with the surrounding test functions in this package.
Source: Linters/SAST tools
Contributor
|
@gangwgr: The following test failed, say
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
2 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.
Summary by CodeRabbit
library-goto the intended source/version for builds that use thisgo.mod.