CNTRLPLANE-2589: Migrate test/e2e-encryption to OpenShift Tests Extension framework#944
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
|
@ropatil010: This pull request references CNTRLPLANE-2589 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughAdds a new encryption E2E test package, rewires registry selection for disruptive encryption suites, and replaces inline test setup with shared helper-based test entrypoints. ChangesEncryption E2E suite and registry wiring
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
test/e2e-encryption/encryption.go (3)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPackage name
e2e_encryptionviolates Go naming convention.The rename from
e2eencryptiontoe2e_encryptionintroduces an underscore. Per coding guidelines, package names must not contain underscores. This same rename also appears inencryption_test.goandmain_test.go. The blank import inmain.gouses the directory path, so the package name isn't constrained by OTE and can remain underscore-free (e.g.e2eencryption).As per coding guidelines: "Do not use uppercase characters, underscores, or dashes in package names."
🤖 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.go` at line 1, The package name was changed to use an underscore, which violates Go package naming conventions. Update the package declaration in the e2e encryption test files to use a lowercase, underscore-free name such as the original e2eencryption, and keep the blank import in main.go aligned with that package name where needed. Make the change consistently across the encryption test package files so the package identifier matches the coding guidelines.Source: Coding guidelines
29-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDe-duplicate the shared
BasicScenarioand simplify constant strings.The identical
BasicScenarioliteral is repeated in all three helpers. Thefmt.Sprintf("encryption-config-openshift-oauth-apiserver")calls have no format directives, and the label selector is a concatenation of constants — both can be plain string literals. Extract a small helper.♻️ Proposed refactor
+func basicScenario() library.BasicScenario { + return 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, + } +} + func testEncryptionTypeIdentity(ctx context.Context, tt testing.TB) { - library.TestEncryptionTypeIdentity(ctx, tt, library.BasicScenario{ - Namespace: "openshift-config-managed", - LabelSelector: "encryption.apiserver.operator.openshift.io/component" + "=" + "openshift-oauth-apiserver", - EncryptionConfigSecretName: fmt.Sprintf("encryption-config-openshift-oauth-apiserver"), - EncryptionConfigSecretNamespace: "openshift-config-managed", - OperatorNamespace: "openshift-authentication-operator", - TargetGRs: library.AuthTargetGRs, - AssertFunc: library.AssertTokens, - }) + library.TestEncryptionTypeIdentity(ctx, tt, basicScenario()) } func testEncryptionTypeUnset(ctx context.Context, tt testing.TB) { - library.TestEncryptionTypeUnset(ctx, tt, library.BasicScenario{ - Namespace: "openshift-config-managed", - LabelSelector: "encryption.apiserver.operator.openshift.io/component" + "=" + "openshift-oauth-apiserver", - EncryptionConfigSecretName: fmt.Sprintf("encryption-config-openshift-oauth-apiserver"), - EncryptionConfigSecretNamespace: "openshift-config-managed", - OperatorNamespace: "openshift-authentication-operator", - TargetGRs: library.AuthTargetGRs, - AssertFunc: library.AssertTokens, - }) + library.TestEncryptionTypeUnset(ctx, tt, basicScenario()) }
fmtcan then be dropped from imports.🤖 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.go` around lines 29 - 75, The three helpers testEncryptionTypeIdentity, testEncryptionTypeUnset, and testEncryptionTurnOnAndOff repeat the same BasicScenario setup and use unnecessary fmt.Sprintf and string concatenation for fixed values. Extract a shared helper that returns the common library.BasicScenario, replace the label selector and encryption config secret name with plain string literals, and update the call sites to reuse that helper; once fmt is no longer needed, remove it from the imports.
72-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
configv1.EncryptionTypeAESCBChere. The named constant avoids a raw string cast and keeps this typo-safe.🤖 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.go` at line 72, The APIServerEncryption type in the encryption test is using a raw string cast instead of the typed constant. Update the `APIServerEncryption` initialization in the encryption setup to use `configv1.EncryptionTypeAESCBC` so the `configv1.APIServerEncryption` value is type-safe and consistent with the rest of the code.
🤖 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.go`:
- Line 1: The package name was changed to use an underscore, which violates Go
package naming conventions. Update the package declaration in the e2e encryption
test files to use a lowercase, underscore-free name such as the original
e2eencryption, and keep the blank import in main.go aligned with that package
name where needed. Make the change consistently across the encryption test
package files so the package identifier matches the coding guidelines.
- Around line 29-75: The three helpers testEncryptionTypeIdentity,
testEncryptionTypeUnset, and testEncryptionTurnOnAndOff repeat the same
BasicScenario setup and use unnecessary fmt.Sprintf and string concatenation for
fixed values. Extract a shared helper that returns the common
library.BasicScenario, replace the label selector and encryption config secret
name with plain string literals, and update the call sites to reuse that helper;
once fmt is no longer needed, remove it from the imports.
- Line 72: The APIServerEncryption type in the encryption test is using a raw
string cast instead of the typed constant. Update the `APIServerEncryption`
initialization in the encryption setup to use `configv1.EncryptionTypeAESCBC` so
the `configv1.APIServerEncryption` value is type-safe and consistent with the
rest of the code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4c22afe2-7056-40af-93c8-7d6496d331c8
📒 Files selected for processing (4)
cmd/cluster-authentication-operator-tests-ext/main.gotest/e2e-encryption/encryption.gotest/e2e-encryption/encryption_test.gotest/e2e-encryption/main_test.go
|
/test e2e-aws-operator-encryption-perf-serial-ote-1of2 |
ropatil010
force-pushed
the
test-e2e-encryption
branch
from
30806c4 to
294aa88
Compare
ropatil010
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
|
PASS Logs: |
liouk
left a comment
liouk
left a comment
There was a problem hiding this comment.
Some minor findings, lgtm otherwise.
ropatil010
force-pushed
the
test-e2e-encryption
branch
2 times, most recently
from
b18e965 to
15c7b8e
Compare
This commit migrates the encryption tests to support both traditional go test and the OpenShift Tests Extension (OTE) framework, following the dual-mode pattern established in PR openshift#926 and openshift#943. Changes: - Add encryption.go with Ginkgo test registration - Refactor encryption_test.go to call shared test functions - Register operator-encryption/serial suite in OTE main.go - Update package name to e2e_encryption (with underscores) The tests now use testing.TB interface for framework compatibility. Suite details: - Name: openshift/cluster-authentication-operator/operator-encryption/serial - Parallelism: 1 (serial execution) - ClusterStability: Disruptive (triggers API server rollouts) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
ropatil010
force-pushed
the
test-e2e-encryption
branch
from
15c7b8e to
f66cb12
Compare
|
Hi @liouk PTAL on the PR. This PR is for OTE migration of test/e2e-encryption folder which has below cases and got PASSED in the CI logs The below CI failures has been fixed in the PR: #945 Failure cases which are irrelevant to my PR changes: |
|
/verified by CI runs |
openshift-ci-robot
added
the
verified
|
@ropatil010: 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. |
|
@ropatil010, |
|
@ropatil010: The following tests 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. |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/unhold We can ignore the CI failure e2e-aws-operator-parallel-ote, e2e-aws-operator-serial-ote. They got pass in CI profile:
|
openshift-ci
Bot
removed
the
do-not-merge/hold
|
Planning to merge PR: 945 first, |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: liouk 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
|
/unhold |
openshift-ci
Bot
removed
the
do-not-merge/hold
openshift-merge-bot
Bot
merged commit cc6815a
into
openshift:master
4 participants
Hi Team,
Migrate the encryption tests to support both traditional go test and the OpenShift Tests Extension (OTE) framework
Changes:
The tests now use testing.TB interface for framework compatibility.
Suite details:
Summary by CodeRabbit