NE-2292: Tests for gatewayAPIWithoutOLM featuregate graduation#30905
NE-2292: Tests for gatewayAPIWithoutOLM featuregate graduation#30905rhamini3 wants to merge 3 commits intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Skipping CI for Draft Pull Request. |
|
@rhamini3: This pull request references Ne-2292 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 "4.22.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. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🚫 Review skipped — only excluded labels are configured. (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
WalkthroughAdds GatewayAPIWithoutOLM branches to Gateway API controller tests: new constants, conditional capability gating, gated test cases validating GatewayClass conditions/finalizer, resource presence/absence, istiod checks, updated cleanup flows, helper functions, and switches GIE verification to read an istiod env var; also exposes a new CLI method for the apiextensions v1 client. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
rhamini3
changed the title
|
@rhamini3: This pull request references NE-2292 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 "4.22.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. |
|
/testwith openshift/origin/main/e2e-gcp-ovn-techpreview openshift/cluster-ingress-operator#1354 |
|
CC @gcs278 |
|
If you are ready for comments, code rabbit might be worth looking at: |
|
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/router/gatewayapicontroller.go`:
- Around line 1340-1355: In istioManagedCRDs, ensure you fail when no *.istio.io
CRDs are found and fix the label lookup logic: iterate crdList.Items and count
matches for strings.Contains(crd.Name, "istio.io"); after the loop, if count ==
0 return an error (e.g., fmt.Errorf("no istio CRDs found")); inside the loop use
the correct map lookup syntax value, ok :=
crd.Labels["ingress.operator.openshift.io/owned"] (not err) and check ok &&
value == "true" before logging, otherwise call e2e.Failf for that CRD.
- Around line 55-59: The test completion keys gatewayClassConditions,
gatewayClassFinalizer, noOLMResourcesPresent, istioCRDsManagedbyCIO, and
noSailoperatorCRDs need to be added to the shared testNames collection so
markTestDone can signal completion for the new GatewayAPIWithoutOLM specs;
update the testNames variable (the list used by checkAllTestsDone()) to include
these five new constants and ensure markTestDone and checkAllTestsDone reference
that same collection so the AfterEach cleanup path for GatewayClass/Istio runs
when those tests are selected.
- Around line 239-246: The current checks use generic type-only queries which
can succeed if any same-type resource exists elsewhere; update the calls so they
explicitly query the expected namespace and assert zero results or absence of
the specific operator objects: change calls to checkIfResourceExists to target
the namespace (expectedSubscriptionNamespace) and verify there are no
subscriptions, CSVs or pods in that namespace (e.g., use a namespaced get/list
and assert count==0), and replace the Istio check that runs
oc.AsAdmin().Run("get").Args("istio") with a check for the Istio operator CRD/CR
kind (e.g., get the istio.operator.openshift.io CRD or list
istio.operator.openshift.io resources) and assert the CRD is absent or that no
Istio CR instances exist (zero items) so the test cannot be fooled by a present
CRD with zero resources.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f88f54c5-b18f-43be-a734-c2e4f9d28f28
📒 Files selected for processing (2)
test/extended/router/gatewayapicontroller.gotest/extended/util/client.go
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
Scheduling required tests: |
|
/testwith openshift/origin/main/e2e-gcp-ovn-techpreview openshift/cluster-ingress-operator#1354 |
|
/retest |
|
/testwith openshift/origin/main/e2e-gcp-ovn-techpreview openshift/cluster-ingress-operator#1354 |
|
Scheduling required tests: |
|
/testwith openshift/origin/main/e2e-gcp-ovn-techpreview openshift/cluster-ingress-operator#1354 |
|
I'm not sure if your changes from last night passed, but now that openshift/cluster-ingress-operator#1354 is merged, we don't have to use testwith: /test e2e-gcp-ovn-techpreview |
|
/test e2e-gcp-ovn-techpreview |
is the change instant, or do we need to wait for a nightly image to contain the PR changes? |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/extended/router/gatewayapicontroller.go (1)
1290-1293: Consider handling additional error message variants for robustness.Per the earlier review discussion,
occan produce"no matches for"on some versions when resource types are unregistered. Adding this variant would improve robustness across different cluster configurations.Suggested improvement
// If the CRD doesn't exist (OLM not installed), that's fine - no resources exist - if err != nil && strings.Contains(err.Error(), "the server doesn't have a resource type") { + if err != nil && (strings.Contains(err.Error(), "the server doesn't have a resource type") || + strings.Contains(err.Error(), "no matches for")) { return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/extended/router/gatewayapicontroller.go` around lines 1290 - 1293, The current error check only looks for "the server doesn't have a resource type" which misses other oc/kubectl variants; update the conditional that uses err and strings.Contains(err.Error(), ...) to also check for "no matches for" (e.g. using || to check strings.Contains(err.Error(), "no matches for") or normalize the message and search for both substrings) so the block in gatewayapicontroller.go that returns on unregistered CRDs handles both "the server doesn't have a resource type" and "no matches for" error messages.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/extended/router/gatewayapicontroller.go`:
- Line 66: The constant noOLMResourcesPresent has a typo in its string value
("no-olm-reources"); update the value to "no-olm-resources" so the annotation
key is spelled correctly, ensuring anywhere that uses noOLMResourcesPresent
(e.g., test completion tracking) gets the corrected annotation string.
---
Nitpick comments:
In `@test/extended/router/gatewayapicontroller.go`:
- Around line 1290-1293: The current error check only looks for "the server
doesn't have a resource type" which misses other oc/kubectl variants; update the
conditional that uses err and strings.Contains(err.Error(), ...) to also check
for "no matches for" (e.g. using || to check strings.Contains(err.Error(), "no
matches for") or normalize the message and search for both substrings) so the
block in gatewayapicontroller.go that returns on unregistered CRDs handles both
"the server doesn't have a resource type" and "no matches for" error messages.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 246ef984-3774-47c7-a07e-255375a18bc7
📒 Files selected for processing (1)
test/extended/router/gatewayapicontroller.go
|
Scheduling required tests: |
This includes 5 unique tests which are to be used to graduate the featuregate from techpreview to GA. There are some tests which fall common within gatewayAPIController and GatewayAPIWithoutOLM. JIRA: https://redhat.atlassian.net/browse/NE-2292
|
@rhamini3: This pull request references NE-2292 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 "4.22.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. |
|
/test e2e-gcp-ovn-techpreview |
|
Scheduling required tests: |
|
/test e2e-gcp-ovn-techpreview |
|
Excellent work @rhamini3! we need to get this in ASAP so we can have 7 days of CI signal before pencils down. /approve |
|
Scheduling required tests: |
| } | ||
|
|
||
| func assertIstioCRDsOwnedByCIO(oc *exutil.CLI) error { | ||
| crdList, err := oc.ApiextensionsV1().CustomResourceDefinitions().List(context.Background(), metav1.ListOptions{}) |
There was a problem hiding this comment.
You should use this and you can remove your changes to test/extended/util/client.go:
| crdList, err := oc.ApiextensionsV1().CustomResourceDefinitions().List(context.Background(), metav1.ListOptions{}) | |
| crdList, err := oc.AdminApiextensionsClient().ApiextensionsV1().CustomResourceDefinitions().List(context.Background(), metav1.ListOptions{}) |
|
New changes are detected. LGTM label has been removed. |
|
/test e2e-gcp-ovn-techpreview |
openshift-ci
bot
added
the
approved
|
Okay - now that we removed the dependency on /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gcs278, rhamini3 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 |
|
Scheduling required tests: |
|
Had a successful run run once more /test e2e-gcp-ovn-techpreview |
|
Green test runs on |
3 participants
This includes 5 unique tests which are to be used to graduate
the featuregate from techpreview to GA. There are some tests
which fall common within gatewayAPIController and
GatewayAPIWithoutOLM.