ESO-424: Restructures e2e test labeling, default filter, and prerequisite handling#161
ESO-424: Restructures e2e test labeling, default filter, and prerequisite handling#161bharath-b-rh wants to merge 1 commit into
Conversation
…site handling Signed-off-by: Bharath B <bhb@redhat.com>
openshift-ci-robot
added
the
jira/valid-reference
|
@bharath-b-rh: This pull request references ESO-424 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 story 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. |
WalkthroughDefault E2E label filtering, suite labels, and README guidance were updated for the Platform, Provider, API, and Feature taxonomy. AWS and custom network policy scenarios now fetch credentials or assert the generated policy shape. Bitwarden tests share readiness and curl-job helpers, and trusted CA bundle checks branch on OpenShift proxy configuration. ChangesE2E label filtering and test wiring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)Error: build linters: unable to load custom analyzer "kubeapilinter": bin/kube-api-linter.so, plugin: not implemented Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bharath-b-rh 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
![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 (2)
test/e2e/README.md (1)
44-46: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: add a language to these fenced code blocks.
markdownlint (MD040) flags the blocks at Line 44 (filter expression) and Line 190 (artifact tree). Adding a hint like
textsilences the warning.📝 Suggested tweak
-``` +```text Platform: isSubsetOf {AWS,Generic} && !(Feature: containsAny {Proxy, Upgrade}) && !(Provider: containsAny Bitwarden)(apply the same to the `_output/e2e-artifacts/...` block at Line 190) </details> Also applies to: 190-195 <details> <summary>🤖 Prompt for AI Agents</summary>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/README.mdaround lines 44 - 46, The fenced code blocks in the
README need a language hint to satisfy markdownlint MD040. Update the two
affected fenced blocks in the e2e README, including the filter expression block
and the_output/e2e-artifacts/...artifact tree block, by adding an
appropriate info string such as text while keeping the content unchanged.</details> <!-- cr-comment:v1:3766293b9690f4a37fdb8e31 --> _Source: Linters/SAST tools_ </blockquote></details> <details> <summary>test/e2e/trusted_ca_bundle_test.go (1)</summary><blockquote> `469-488`: _📐 Maintainability & Code Quality_ | _🔵 Trivial_ | _⚡ Quick win_ **Use the literal proxy singleton name `"cluster"` instead of `common.ExternalSecretsConfigObjectName`.** The cluster-wide OpenShift proxy is a singleton named `cluster`. This lookup currently works only because `ExternalSecretsConfigObjectName` happens to equal `"cluster"`. Coupling the proxy name to an unrelated ESC constant is misleading and silently breaks proxy detection if that constant ever changes, which would flip the test to the wrong assertion branch. <details> <summary>♻️ Proposed change</summary> ```diff - proxyCR, err := suiteDynamicClient.Resource(proxyGVR).Get(ctx, common.ExternalSecretsConfigObjectName, metav1.GetOptions{}) + // The cluster-wide OpenShift proxy is a singleton named "cluster". + proxyCR, err := suiteDynamicClient.Resource(proxyGVR).Get(ctx, "cluster", metav1.GetOptions{})🤖 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/trusted_ca_bundle_test.go` around lines 469 - 488, The OpenShift proxy lookup in isOpenShiftClusterProxyConfigured is coupled to an unrelated constant; replace common.ExternalSecretsConfigObjectName with the literal singleton name "cluster" when calling suiteDynamicClient.Resource(proxyGVR).Get so the proxy detection logic remains correct even if the ESC constant changes.
🤖 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/bitwarden_api_test.go`:
- Around line 51-60: The Bitwarden API suite setup in BeforeAll mutates cluster
state via ensureBitwardenOperandReady and copies bitwarden-creds into
utils.BitwardenOperandNamespace, but there is no teardown to revert those
changes. Add an AfterAll in bitwarden_api_test.go, mirroring the cleanup pattern
from bitwarden_es_test.go, to restore the ExternalSecretsConfig/plugin and
egress-policy state and delete the copied secret after the suite finishes.
In `@test/e2e/trusted_ca_bundle_test.go`:
- Around line 350-371: The deployment fetch assertions inside the trusted CA
bundle tests use bare g.Expect(err).NotTo(HaveOccurred()) calls without any
failure context, so update the Eventually blocks in trusted_ca_bundle_test.go to
include meaningful messages for the Get call failures. Use the existing
Deployment fetches around externalsecrets.OperandCoreControllerDeployment and
the surrounding g.Expect/Eventually pattern in this test to add a clear message
describing which namespace/deployment lookup failed, matching the style used
elsewhere in the file.
---
Nitpick comments:
In `@test/e2e/README.md`:
- Around line 44-46: The fenced code blocks in the README need a language hint
to satisfy markdownlint MD040. Update the two affected fenced blocks in the e2e
README, including the filter expression block and the
`_output/e2e-artifacts/...` artifact tree block, by adding an appropriate info
string such as text while keeping the content unchanged.
In `@test/e2e/trusted_ca_bundle_test.go`:
- Around line 469-488: The OpenShift proxy lookup in
isOpenShiftClusterProxyConfigured is coupled to an unrelated constant; replace
common.ExternalSecretsConfigObjectName with the literal singleton name "cluster"
when calling suiteDynamicClient.Resource(proxyGVR).Get so the proxy detection
logic remains correct even if the ESC constant changes.
🪄 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: 39ca9028-8fba-41e2-9b11-313c760a5da9
📒 Files selected for processing (10)
Makefiletest/e2e/README.mdtest/e2e/bitwarden_api_test.gotest/e2e/bitwarden_es_test.gotest/e2e/bitwarden_helpers_test.gotest/e2e/e2e_test.gotest/e2e/trusted_ca_bundle_test.gotest/utils/bitwarden.gotest/utils/bitwarden_api_runner.gotest/utils/conditions.go
|
/woof |
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 kubernetes-sigs/prow repository. |
|
/test e2e-operator |
|
@bharath-b-rh: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #161 +/- ##
==========================================
+ Coverage 46.19% 46.32% +0.12%
==========================================
Files 31 31
Lines 5451 5464 +13
==========================================
+ Hits 2518 2531 +13
Misses 2626 2626
Partials 307 307
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/cherrypick release-1.2 |
|
@bharath-b-rh: once the present PR merges, I will cherry-pick it on top of 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 kubernetes-sigs/prow repository. |
|
Adding below labels based on the e2e results |
openshift-ci
Bot
added
qe-approved
4 participants
Summary
Platform,Provider,Feature,API) across e2e suites and update the Makefile default filter to run portable + AWS tests while excluding Bitwarden provider, proxy, and upgrade checks.Skip()calls: selected specs now fail fast when prerequisites (secrets, cluster config) are missing; label filters are the opt-in mechanism.bitwarden-tls-certsand enable the SDK server plugin viaensureBitwardenOperandReady; onlyProvider:Bitwardenspecs require pre-provisionedbitwarden-creds.test/e2e/README.mdwith label keys, filter recipes, prerequisites, and local/CI artifact output (ARTIFACT_DIR/_output).Changes
Labeling
Feature:*labels to previously unlabeled contexts (OverrideEnv,RevisionHistoryLimit,UnsafeAllowGenericTargets,CustomAnnotations,NetworkPolicy,TrustedCABundle, etc.).CrossPlatform:GCP-AWS,Proxy:HTTP,PostUpgradeCheck,Suite:Bitwarden) withPlatform:GCP+Provider:AWS,Feature:Proxy,Feature:Upgrade, etc.Provider:Bitwardenso default CI runs health/auth only.Makefile
Platform: isSubsetOf {AWS,Generic} && !(Feature: containsAny {Proxy, Upgrade}) && !(Provider: containsAny Bitwarden){Proxy, Upgrade}brace syntax so Ginkgo parsescontainsAnycorrectly (comma is otherwise treated as OR).Test behavior
Expectfailures forProvider:AWS,Platform:GCP, andProvider:Bitwarden.eso-user-*creation (no CR removal — CEL immutability).specandstatusonproxy.config.openshift.io/cluster).test/e2e/bitwarden_helpers_test.go; addRunBitwardenCurlJobfor cred-less API health/auth tests.Test plan