NO-JIRA: update func from library-go#724
Open
gangwgr wants to merge 2 commits into
Open
Conversation
Contributor
|
Skipping CI for Draft Pull Request. |
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
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:
WalkthroughThis PR bumps ChangesEncryption test migration
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 3 warnings, 1 inconclusive)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
Author
|
/test e2e-aws-operator-encryption-kms |
![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
🧹 Nitpick comments (1)
test/e2e-encryption-kms/encryption_kms.go (1)
60-65: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse the existing test context and callback inputs here.
context.TODO()drops cancellation from the enclosing test, and reopening clients inside the callback is unnecessary when the scenario already passes them in. As per path instructions,Go security (prodsec-skills): context.Context for cancellation and timeouts.Suggested fix
- CreateResourceFunc: func(t testing.TB, _ library.ClientSet, namespace string) runtime.Object { - return library.CreateAndStoreRouteOfLife(context.TODO(), t, library.GetClients(t), ns) + CreateResourceFunc: func(t testing.TB, cs library.ClientSet, namespace string) runtime.Object { + return library.CreateAndStoreRouteOfLife(ctx, t, cs, namespace) }, AssertResourceEncryptedFunc: library.AssertRouteOfLifeEncrypted, AssertResourceNotEncryptedFunc: library.AssertRouteOfLifeNotEncrypted, - ResourceFunc: func(t testing.TB, _ string) runtime.Object { return library.RouteOfLife(ns) }, + ResourceFunc: func(t testing.TB, namespace string) runtime.Object { return library.RouteOfLife(namespace) },Also applies to: 97-102
🤖 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 60 - 65, The resource creation callback is ignoring the existing test context and passed-in client set by using context.TODO() and reopening clients via library.GetClients(t), which drops cancellation and duplicates work. Update the CreateResourceFunc closures for RouteOfLife to use the callback’s testing.TB and provided library.ClientSet/context inputs instead of creating new clients, and thread the existing context through the call to library.CreateAndStoreRouteOfLife so cancellation and timeouts are preserved.Source: Path instructions
🤖 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 147: The go.mod replace for github.com/openshift/library-go currently
points to a personal fork, which should be removed or justified. Update the
module dependency so it uses the upstream OpenShift-maintained
github.com/openshift/library-go chain again, and drop the replace entry once the
helper change is upstreamed; if the fork must remain temporarily, document the
fork owner and synchronization plan near the dependency management area.
In `@test/e2e-encryption-kms/encryption_kms.go`:
- Line 48: The deferred namespace cleanup in the e2e KMS test drops the error
returned by Namespace Delete, so update the defer around
cs.Kube.CoreV1().Namespaces().Delete to check and handle any delete failure
instead of ignoring it; apply the same fix to the other deferred namespace
delete in this test file as well.
---
Nitpick comments:
In `@test/e2e-encryption-kms/encryption_kms.go`:
- Around line 60-65: The resource creation callback is ignoring the existing
test context and passed-in client set by using context.TODO() and reopening
clients via library.GetClients(t), which drops cancellation and duplicates work.
Update the CreateResourceFunc closures for RouteOfLife to use the callback’s
testing.TB and provided library.ClientSet/context inputs instead of creating new
clients, and thread the existing context through the call to
library.CreateAndStoreRouteOfLife so cancellation and timeouts are preserved.
🪄 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: a0604f00-b226-45d9-93f8-f19eb459cd3a
⛔ 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 (4)
_output/golang-versions_output/named-golang-versionsgo.modtest/e2e-encryption-kms/encryption_kms.go
| _, err := cs.Kube.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{}) | ||
| defer cs.Kube.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Check the deferred namespace delete error.
Delete failures are currently dropped, which can leak namespaces and make later e2e runs flaky. As per path instructions, Go security (prodsec-skills): Never ignore error returns.
Suggested fix
- defer cs.Kube.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{})
+ t.Cleanup(func() {
+ require.NoError(t, cs.Kube.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{}))
+ })Also applies to: 85-85
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 48-48: Error return value of (k8s.io/client-go/kubernetes/typed/core/v1.NamespaceInterface).Delete is not checked
(errcheck)
🤖 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 48, The deferred namespace
cleanup in the e2e KMS test drops the error returned by Namespace Delete, so
update the defer around cs.Kube.CoreV1().Namespaces().Delete to check and handle
any delete failure instead of ignoring it; apply the same fix to the other
deferred namespace delete in this test file as well.
Sources: Path instructions, Linters/SAST tools
ardaguclu
reviewed
Jun 29, 2026
| ResourceFunc: func(t testing.TB, _ string) runtime.Object { return operatorencryption.RouteOfLife(t, ns) }, | ||
| AssertResourceEncryptedFunc: library.AssertRouteOfLifeEncrypted, | ||
| AssertResourceNotEncryptedFunc: library.AssertRouteOfLifeNotEncrypted, | ||
| ResourceFunc: func(t testing.TB, _ string) runtime.Object { return library.RouteOfLife(ns) }, |
Member
There was a problem hiding this comment.
We need to update RouteOfLife to accept t.
Member
|
We are making changes for all encryption types. It would be better to run all encryption jobs. |
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
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 60-62: The RouteOfLife resource creation is discarding the test
context, so the client calls won’t respect Ginkgo cancellation and timeout
behavior. Update the CreateResourceFunc and the RouteOfLife setup path to thread
through the existing ctx instead of creating a fresh context, using the same
context value when calling library.CreateAndStoreRouteOfLife and any related
client helpers. Make the fix in the test helpers around CreateResourceFunc and
the RouteOfLife creation call so context is consistently propagated.
🪄 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: 485d3a0b-3dca-404e-b4de-1bbb947a794f
⛔ 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 (4)
_output/golang-versions_output/named-golang-versionsgo.modtest/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)
- go.mod
Comment on lines
60
to
62
| CreateResourceFunc: func(t testing.TB, _ library.ClientSet, namespace string) runtime.Object { | ||
| return operatorencryption.CreateAndStoreRouteOfLife(context.TODO(), t, operatorencryption.GetClients(t), ns) | ||
| return library.CreateAndStoreRouteOfLife(context.TODO(), t, library.GetClients(t), ns) | ||
| }, |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Propagate the test context into RouteOfLife creation.
Line 61 and Line 98 discard the test ctx, so these client calls won't honor the Ginkgo cancellation/timeout path.
Suggested fix
- return library.CreateAndStoreRouteOfLife(context.TODO(), t, library.GetClients(t), ns)
+ return library.CreateAndStoreRouteOfLife(ctx, t, library.GetClients(t), ns)As per path instructions, Go security (prodsec-skills): context.Context for cancellation and timeouts.
Also applies to: 97-99
🤖 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 60 - 62, The
RouteOfLife resource creation is discarding the test context, so the client
calls won’t respect Ginkgo cancellation and timeout behavior. Update the
CreateResourceFunc and the RouteOfLife setup path to thread through the existing
ctx instead of creating a fresh context, using the same context value when
calling library.CreateAndStoreRouteOfLife and any related client helpers. Make
the fix in the test helpers around CreateResourceFunc and the RouteOfLife
creation call so context is consistently propagated.
Source: Path instructions
openshift-ci
Bot
added
the
needs-rebase
openshift-ci
Bot
removed
the
needs-rebase
gangwgr
changed the title
openshift-ci-robot
added
the
jira/valid-reference
|
@gangwgr: This pull request explicitly references no 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. |
Contributor
Author
|
/retest |
Member
|
Overall sounds good to me. But we need to see all the jobs green. |
ardaguclu
reviewed
Jul 1, 2026
| "k8s.io/apimachinery/pkg/runtime" | ||
|
|
||
| "github.com/openshift/cluster-openshift-apiserver-operator/pkg/operator/operatorclient" | ||
| operatorencryption "github.com/openshift/cluster-openshift-apiserver-operator/test/library/encryption" |
Member
There was a problem hiding this comment.
Can we remove unused files in second commit?.
| operatorv1client "github.com/openshift/client-go/operator/clientset/versioned/typed/operator/v1" | ||
| "github.com/openshift/cluster-openshift-apiserver-operator/pkg/operator/operatorclient" | ||
| operatorencryption "github.com/openshift/cluster-openshift-apiserver-operator/test/library/encryption" | ||
| testlibrary "github.com/openshift/cluster-openshift-apiserver-operator/test/library" |
Member
There was a problem hiding this comment.
Can we get rid of this dependency?
gangwgr
force-pushed
the
bump-test-cases
branch
2 times, most recently
from
719519f to
5c86635
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/e2e-encryption/encryption_test.go (1)
63-71: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
CreateResourceFuncignores the injectedClientSetand builds a new one instead.
CreateResourceFuncreceives alibrary.ClientSetargument but discards it (_) and callslibrary.GetClients(t)again, even thoughcsis already available in the enclosing scope (line 49). This creates a redundant client for no benefit and diverges from the parameter contract the scenario framework provides.♻️ Proposed fix
- CreateResourceFunc: func(t testing.TB, _ library.ClientSet, namespace string) runtime.Object { - return library.CreateAndStoreRouteOfLife(context.TODO(), t, library.GetClients(t), ns) + CreateResourceFunc: func(t testing.TB, clientSet library.ClientSet, namespace string) runtime.Object { + return library.CreateAndStoreRouteOfLife(context.TODO(), t, clientSet, 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/encryption_test.go` around lines 63 - 71, CreateResourceFunc is ignoring the injected ClientSet and unnecessarily calling library.GetClients again. Update the CreateResourceFunc closure to use the provided client set parameter instead of discarding it, matching the scenario contract and avoiding redundant client creation. Keep the fix localized to the CreateResourceFunc and, if needed, the related ResourceFunc usage in this test setup.test/e2e-encryption-rotation/e2e-encryption-rotation_test.go (1)
58-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
CreateResourceFuncignores its providedClientSet, inconsistent withGetRawResourceFunc.
CreateResourceFuncdiscards the injectedlibrary.ClientSetand callslibrary.GetClients(t)again to build a new client, whereas the siblingGetRawResourceFunccorrectly uses theclientSetparameter it receives. This inconsistency creates an unnecessary redundant client and diverges from the pattern used two lines below.♻️ Proposed fix
- CreateResourceFunc: func(t testing.TB, _ library.ClientSet, _ string) runtime.Object { - return library.CreateAndStoreRouteOfLife(ctx, t, library.GetClients(t), ns) + CreateResourceFunc: func(t testing.TB, clientSet library.ClientSet, _ string) runtime.Object { + return library.CreateAndStoreRouteOfLife(ctx, t, clientSet, 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-rotation/e2e-encryption-rotation_test.go` around lines 58 - 65, CreateResourceFunc is ignoring the injected ClientSet and rebuilding clients with library.GetClients(t), which is inconsistent with GetRawResourceFunc. Update the CreateResourceFunc closure in e2e-encryption-rotation_test.go to use its clientSet parameter and pass that through to library.CreateAndStoreRouteOfLife, keeping the resource creation path aligned with the existing ClientSet-based pattern.
🤖 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-rotation/e2e-encryption-rotation_test.go`:
- Around line 29-49: The namespace cleanup and operator config update in the
test need to handle errors robustly. In the deferred Namespace Delete call after
creating ns, capture and check the delete error instead of ignoring it. In
updateUnsupportedConfig, replace the plain Get→mutate→Update flow on
operatorClient.OpenShiftAPIServers().Get/Update with a retry-on-conflict loop so
updates to the OpenShiftAPIServer “cluster” object are retried if Spec changes
between reads. Use the existing updateUnsupportedConfig helper and the
operatorClient/OpenShiftAPIServers symbols to keep the fix localized.
In `@test/e2e-encryption/encryption_test.go`:
- Around line 48-54: The deferred namespace cleanup in the encryption e2e test
ignores the error returned by cs.Kube.CoreV1().Namespaces().Delete, which
violates errcheck expectations. Update the defer after the namespace Create call
to handle the Delete return value explicitly, using the same ctx and namespace
identifier in the test setup, and route any cleanup failure through the test
helper/assertion pattern used in encryption_test.go so the error is not silently
discarded.
---
Nitpick comments:
In `@test/e2e-encryption-rotation/e2e-encryption-rotation_test.go`:
- Around line 58-65: CreateResourceFunc is ignoring the injected ClientSet and
rebuilding clients with library.GetClients(t), which is inconsistent with
GetRawResourceFunc. Update the CreateResourceFunc closure in
e2e-encryption-rotation_test.go to use its clientSet parameter and pass that
through to library.CreateAndStoreRouteOfLife, keeping the resource creation path
aligned with the existing ClientSet-based pattern.
In `@test/e2e-encryption/encryption_test.go`:
- Around line 63-71: CreateResourceFunc is ignoring the injected ClientSet and
unnecessarily calling library.GetClients again. Update the CreateResourceFunc
closure to use the provided client set parameter instead of discarding it,
matching the scenario contract and avoiding redundant client creation. Keep the
fix localized to the CreateResourceFunc and, if needed, the related ResourceFunc
usage in this test setup.
🪄 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: 06957471-4f7a-4f4b-8e06-328d65a60811
⛔ Files ignored due to path filters (24)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/client-go/route/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/localobjectreference.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/route.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routehttpheader.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routehttpheaderactions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routehttpheaderactionunion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routehttpheaders.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routeingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routeingresscondition.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routeport.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routesethttpheader.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routespec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routestatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routetargetreference.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/tlsconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/clientset/versioned/scheme/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/clientset/versioned/scheme/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/clientset/versioned/typed/route/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/clientset/versioned/typed/route/v1/generated_expansion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/clientset/versioned/typed/route/v1/route.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/clientset/versioned/typed/route/v1/route_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/scenarios.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (6)
go.modtest/e2e-encryption-rotation/e2e-encryption-rotation_test.gotest/e2e-encryption/encryption_test.gotest/e2e/operator_test.gotest/library/encryption/assertion.gotest/library/encryption/helpers.go
💤 Files with no reviewable changes (2)
- test/library/encryption/assertion.go
- test/library/encryption/helpers.go
Comment on lines
+29
to
49
| cs := library.GetClients(t) | ||
|
|
||
| ns := fmt.Sprintf("test-encryption-on-off-%s", rand.String(4)) | ||
| _, err := cs.KubeClient.CoreV1().Namespaces().Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}}, metav1.CreateOptions{}) | ||
| _, err := cs.Kube.CoreV1().Namespaces().Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}}, metav1.CreateOptions{}) | ||
| require.NoError(t, err) | ||
| defer cs.Kube.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{}) | ||
|
|
||
| kubeConfig, err := librarygo.NewClientConfigForTest() | ||
| require.NoError(t, err) | ||
| operatorClient, err := operatorv1client.NewForConfig(kubeConfig) | ||
| require.NoError(t, err) | ||
| defer cs.KubeClient.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{}) | ||
|
|
||
| updateUnsupportedConfig := func(raw []byte) error { | ||
| cs := operatorencryption.GetClients(t) | ||
| apiServerOperator, err := cs.OperatorClient.Get(ctx, "cluster", metav1.GetOptions{}) | ||
| apiServerOperator, err := operatorClient.OpenShiftAPIServers().Get(ctx, "cluster", metav1.GetOptions{}) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| apiServerOperator.Spec.UnsupportedConfigOverrides.Raw = raw | ||
| _, err = cs.OperatorClient.Update(ctx, apiServerOperator, metav1.UpdateOptions{}) | ||
| _, err = operatorClient.OpenShiftAPIServers().Update(ctx, apiServerOperator, metav1.UpdateOptions{}) | ||
| return err | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unchecked error on deferred namespace delete, and Get/Update without conflict retry.
Two issues in this block:
- Line 34: the deferred
Deleteerror is unchecked (same errcheck finding as inencryption_test.go). Per path instructions, error returns should not be ignored. - Lines 41-49:
updateUnsupportedConfigdoes a plain Get→mutate→Update against theOpenShiftAPIServeroperator object with no conflict retry. If the operator or another actor updatesSpecbetween the Get and Update (a real possibility for a live operator object during a rotation test), theUpdatewill fail with a conflict and the test will flake instead of retrying.
🩹 Proposed fix for the conflict retry
updateUnsupportedConfig := func(raw []byte) error {
- apiServerOperator, err := operatorClient.OpenShiftAPIServers().Get(ctx, "cluster", metav1.GetOptions{})
- if err != nil {
- return err
- }
- apiServerOperator.Spec.UnsupportedConfigOverrides.Raw = raw
- _, err = operatorClient.OpenShiftAPIServers().Update(ctx, apiServerOperator, metav1.UpdateOptions{})
- return err
+ return retry.RetryOnConflict(retry.DefaultRetry, func() error {
+ apiServerOperator, err := operatorClient.OpenShiftAPIServers().Get(ctx, "cluster", metav1.GetOptions{})
+ if err != nil {
+ return err
+ }
+ apiServerOperator.Spec.UnsupportedConfigOverrides.Raw = raw
+ _, err = operatorClient.OpenShiftAPIServers().Update(ctx, apiServerOperator, metav1.UpdateOptions{})
+ return err
+ })
}🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 34-34: Error return value of (k8s.io/client-go/kubernetes/typed/core/v1.NamespaceInterface).Delete is not checked
(errcheck)
🤖 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-rotation/e2e-encryption-rotation_test.go` around lines 29
- 49, The namespace cleanup and operator config update in the test need to
handle errors robustly. In the deferred Namespace Delete call after creating ns,
capture and check the delete error instead of ignoring it. In
updateUnsupportedConfig, replace the plain Get→mutate→Update flow on
operatorClient.OpenShiftAPIServers().Get/Update with a retry-on-conflict loop so
updates to the OpenShiftAPIServer “cluster” object are retried if Spec changes
between reads. Use the existing updateUnsupportedConfig helper and the
operatorClient/OpenShiftAPIServers symbols to keep the fix localized.
Sources: Path instructions, Linters/SAST tools
Comment on lines
48
to
+54
| ctx := context.TODO() | ||
| cs := operatorencryption.GetClients(t) | ||
| cs := library.GetClients(t) | ||
|
|
||
| ns := fmt.Sprintf("test-encryption-on-off-%s", rand.String(4)) | ||
| _, err := cs.KubeClient.CoreV1().Namespaces().Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}}, metav1.CreateOptions{}) | ||
| _, err := cs.Kube.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{}) | ||
| defer cs.Kube.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{}) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Unchecked error on deferred namespace delete.
Static analysis flags the deferred Delete call's error as unchecked (errcheck). Per path instructions, Go code should never ignore error returns.
🩹 Proposed fix
- defer cs.Kube.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{})
+ defer func() {
+ if err := cs.Kube.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{}); err != nil {
+ t.Logf("failed to delete namespace %q: %v", ns, err)
+ }
+ }()📝 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
| ctx := context.TODO() | |
| cs := operatorencryption.GetClients(t) | |
| cs := library.GetClients(t) | |
| ns := fmt.Sprintf("test-encryption-on-off-%s", rand.String(4)) | |
| _, err := cs.KubeClient.CoreV1().Namespaces().Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}}, metav1.CreateOptions{}) | |
| _, err := cs.Kube.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{}) | |
| defer cs.Kube.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{}) | |
| ctx := context.TODO() | |
| cs := library.GetClients(t) | |
| ns := fmt.Sprintf("test-encryption-on-off-%s", rand.String(4)) | |
| _, err := cs.Kube.CoreV1().Namespaces().Create(ctx, &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: ns}}, metav1.CreateOptions{}) | |
| require.NoError(t, err) | |
| defer func() { | |
| if err := cs.Kube.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{}); err != nil { | |
| t.Logf("failed to delete namespace %q: %v", ns, err) | |
| } | |
| }() |
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 54-54: Error return value of (k8s.io/client-go/kubernetes/typed/core/v1.NamespaceInterface).Delete is not checked
(errcheck)
🤖 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/encryption_test.go` around lines 48 - 54, The deferred
namespace cleanup in the encryption e2e test ignores the error returned by
cs.Kube.CoreV1().Namespaces().Delete, which violates errcheck expectations.
Update the defer after the namespace Create call to handle the Delete return
value explicitly, using the same ctx and namespace identifier in the test setup,
and route any cleanup failure through the test helper/assertion pattern used in
encryption_test.go so the error is not silently discarded.
Sources: Path instructions, Linters/SAST tools
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e-encryption/encryption_test.go (1)
67-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant client creation.
library.GetClients(t)is called again here despitecsalready being available in the enclosing scope from Line 49.♻️ Reuse outer client set
- return library.CreateAndStoreRouteOfLife(context.TODO(), t, library.GetClients(t), ns) + return library.CreateAndStoreRouteOfLife(context.TODO(), t, cs, 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/encryption_test.go` at line 67, Reuse the existing client set from the enclosing scope instead of calling library.GetClients(t) again in the CreateAndStoreRouteOfLife path. Update the test logic around the CreateAndStoreRouteOfLife invocation to pass cs directly, keeping the client initialization centralized and avoiding redundant client creation.
🤖 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.
Nitpick comments:
In `@test/e2e-encryption/encryption_test.go`:
- Line 67: Reuse the existing client set from the enclosing scope instead of
calling library.GetClients(t) again in the CreateAndStoreRouteOfLife path.
Update the test logic around the CreateAndStoreRouteOfLife invocation to pass cs
directly, keeping the client initialization centralized and avoiding redundant
client creation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: e5e2aaf6-ee17-4bb9-b68f-f62121413a7a
⛔ Files ignored due to path filters (24)
go.sumis excluded by!**/*.sumvendor/github.com/openshift/client-go/route/applyconfigurations/internal/internal.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/localobjectreference.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/route.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routehttpheader.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routehttpheaderactions.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routehttpheaderactionunion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routehttpheaders.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routeingress.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routeingresscondition.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routeport.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routesethttpheader.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routespec.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routestatus.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/routetargetreference.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/applyconfigurations/route/v1/tlsconfig.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/clientset/versioned/scheme/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/clientset/versioned/scheme/register.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/clientset/versioned/typed/route/v1/doc.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/clientset/versioned/typed/route/v1/generated_expansion.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/clientset/versioned/typed/route/v1/route.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/client-go/route/clientset/versioned/typed/route/v1/route_client.gois excluded by!**/vendor/**,!vendor/**vendor/github.com/openshift/library-go/test/library/encryption/scenarios.gois excluded by!**/vendor/**,!vendor/**vendor/modules.txtis excluded by!**/vendor/**,!vendor/**
📒 Files selected for processing (6)
go.modtest/e2e-encryption-rotation/e2e-encryption-rotation_test.gotest/e2e-encryption/encryption_test.gotest/e2e/operator_test.gotest/library/encryption/assertion.gotest/library/encryption/helpers.go
💤 Files with no reviewable changes (2)
- test/library/encryption/helpers.go
- test/library/encryption/assertion.go
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/operator_test.go
Contributor
Author
|
/test e2e-gcp-operator-encryption-aescbc |
Member
|
/approve |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ardaguclu The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
Contributor
|
/lgtm |
Contributor
Author
|
/verified by ci runs |
|
@gangwgr: This PR has been marked as verified by 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
verified
Contributor
Author
|
/retest |
openshift-ci-robot
removed
the
verified
Contributor
|
/lgtm |
Contributor
Author
|
/test |
Contributor
Author
|
/test e2e-gcp-operator-encryption-rotation-aescbc |
Contributor
Author
|
/test e2e-gcp-operator-encryption-rotation-aesgcm |
Contributor
Author
|
/test e2e-gcp-operator-encryption-aesgcm |
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
4 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
Chores
Tests