Skip to content

NE-2789: Add a guard to skip IngressControllerMultipleHAProxyVersions tests if IngressController CRD lacks haproxyVersion field#31424

Open
rhamini3 wants to merge 1 commit into
openshift:mainfrom
rhamini3:haproxyUpdate
Open

NE-2789: Add a guard to skip IngressControllerMultipleHAProxyVersions tests if IngressController CRD lacks haproxyVersion field#31424
rhamini3 wants to merge 1 commit into
openshift:mainfrom
rhamini3:haproxyUpdate

Conversation

@rhamini3

@rhamini3 rhamini3 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

With the introduction of featuregate IngressControllerMultipleHAProxyVersions origin tests, we want to make sure the tests are backwards compatible and dont start to fail during featuregate promotion.

this change is adding a guard to check the IngressController CRD to confirm the haproxyVersion field is present before trying to run the tests

Summary by CodeRabbit

  • Tests
    • Added a compatibility check to ensure the IngressController custom resource supports HAProxy version configuration.
    • Skips related validation when the installed operator does not yet provide this capability.

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Pipeline controller notification
This repo is configured to use the pipeline controller. Second-stage tests will be triggered either automatically or after lgtm label is added, depending on the repository configuration. The pipeline controller will automatically detect which contexts are required and will utilize /test Prow commands to trigger the second stage.

For optional jobs, comment /test ? to see a list of all defined jobs. To trigger manually all jobs from second stage use /pipeline required command.

This repository is configured in: automatic mode

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jul 23, 2026
@openshift-ci-robot

openshift-ci-robot commented Jul 23, 2026

Copy link
Copy Markdown

@rhamini3: This pull request references NE-2789 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.

Details

In response to this:

With the introduction of featuregate IngressControllerMultipleHAProxyVersions origin tests, we want to make sure the tests are backwards compatible and dont start to fail during featuregate promotion.

this change is adding a guard to check the IngressController CRD to confirm the haproxyVersion field is present before trying to run the tests

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.

@coderabbitai

coderabbitai Bot commented Jul 23, 2026

Copy link
Copy Markdown

Walkthrough

The multi-HAProxy test now inspects the IngressController CRD during setup and skips when the spec.haproxyVersion field is unavailable, preventing version assertions from running against outdated operator schemas.

Changes

HAProxy CRD Compatibility

Layer / File(s) Summary
CRD schema precondition
test/extended/router/multi-haproxy.go
Adds an API extensions client and skips the test when the IngressController CRD lacks spec.haproxyVersion.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • openshift/origin#31392: Both changes update the multi-HAProxy test, with this PR adding a CRD schema precondition.
🚥 Pre-merge checks | ✅ 13 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Test Structure And Quality ⚠️ Warning The new CRD lookup in BeforeEach uses context.Background() with a bare Get, so setup can hang indefinitely and lacks a diagnostic failure message. Wrap the CRD Get in context.WithTimeout/defer cancel, and add a meaningful Expect message for the guard path.
Microshift Test Compatibility ⚠️ Warning The suite uses operator.openshift.io IngressController APIs and an OCPFeatureGate tag, with no MicroShift skip or compatible apigroup tag. Add an [apigroup:operator.openshift.io] or [Skipped:MicroShift] tag, or guard with exutil.IsMicroShiftCluster() if this test must stay non-MicroShift.
✅ Passed checks (13 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Stable And Deterministic Test Names ✅ Passed All Ginkgo Describe/It titles in the changed test file are static and descriptive; none include dynamic or generated values.
Single Node Openshift (Sno) Test Compatibility ✅ Passed The modified HAProxy version tests only create/inspect ingress controllers and router pods; they don’t assume multiple nodes or HA failover, and no SNO guard is needed.
Topology-Aware Scheduling Compatibility ✅ Passed Only a router e2e test changed; it adds a CRD schema guard and no scheduling, manifest, or controller logic was introduced.
Ote Binary Stdout Contract ✅ Passed No new stdout writes in process-level code; the added guard only does CRD reads and g.Skip, with no fmt/log output in init/Describe setup.
Ipv6 And Disconnected Network Test Compatibility ✅ Passed Changed test only adds a CRD schema guard; no hardcoded IPs, IP parsing, public hosts, or external downloads appear in multi-haproxy.go.
No-Weak-Crypto ✅ Passed The PR only adds a CRD schema guard in a test file; no weak-crypto algorithms, custom crypto, or secret comparisons were introduced.
Container-Privileges ✅ Passed The only changed file is a Go test; no container/K8s manifest or privileged settings were introduced.
No-Sensitive-Data-In-Logs ✅ Passed The PR only adds a CRD presence check and a skip message; no passwords, tokens, PII, or other sensitive data are logged.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding a guard to skip the HAProxy version tests when the CRD lacks haproxyVersion.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/extended/router/multi-haproxy.go`:
- Around line 68-69: Update the CRD lookup in BeforeEach to create a context
with a bounded timeout using context.WithTimeout, defer its cancel function, and
pass the timed context to ApiextensionsV1().CustomResourceDefinitions().Get
instead of the unbounded context.Background().
🪄 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 YAML (base), Central YAML (inherited)

Review profile: CHILL

Plan: Enterprise

Run ID: 5074c4b5-db36-4dc8-86b6-3d1d6e46c56d

📥 Commits

Reviewing files that changed from the base of the PR and between c73a0bb and 2ad4877.

📒 Files selected for processing (1)
  • test/extended/router/multi-haproxy.go

Comment thread test/extended/router/multi-haproxy.go
@openshift-ci
openshift-ci Bot requested review from Miciah and davidesalerno July 23, 2026 20:15
@gcs278

gcs278 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

this is a straightforward fix that will allow openshift/api#2947 to pass CI. Thanks!
/approve
/lgtm

But let's only apply the verified by CI until we pass a testwith openshift/api#2947 to validate it works. I can do that.

@gcs278

gcs278 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

/testwith openshift/origin/master/e2e-gcp-ovn openshift/api#2947

@openshift-ci

openshift-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

@gcs278, testwith: could not generate prow job. ERROR:

could not determine ci op config from metadata: got unexpected http 404 status code from configresolver: failed to get config: could not find any config for branch master on repo openshift/origin

@gcs278

gcs278 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

/testwith openshift/origin/main/e2e-gcp-ovn openshift/api#2947

@gcs278

gcs278 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

/testwith openshift/origin/main/e2e-gcp-ovn openshift/api#2947

@openshift-ci openshift-ci Bot added the ready-for-human-review Indicates a PR has been reviewed by automated tools and is ready for human review label Jul 23, 2026
@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2026
@openshift-ci

openshift-ci Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

[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

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 23, 2026
@gcs278

gcs278 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

@rhamini3 just one nit pick about the title, it's skipping if the CRD lacks the field, not if the feature is enabled, maybe change to:

NE-2789: Skip multi-HAProxy tests if IngressController CRD lacks haproxyVersion field

@rhamini3 rhamini3 changed the title NE-2789: Add a guard to skip IngressControllerMultipleHAProxyVersions tests if feature is not enabled NE-2789: Add a guard to skip IngressControllerMultipleHAProxyVersions tests if IngressController CRD lacks haproxyVersion field Jul 23, 2026
@gcs278

gcs278 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

idk why the pipeline hasn't triggered yet.

/pipeline required

@openshift-merge-bot

Copy link
Copy Markdown
Contributor

Scheduling required tests:
/test e2e-aws-csi
/test e2e-aws-ovn-fips
/test e2e-aws-ovn-microshift
/test e2e-aws-ovn-microshift-serial
/test e2e-aws-ovn-serial-1of2
/test e2e-aws-ovn-serial-2of2
/test e2e-gcp-csi
/test e2e-gcp-ovn
/test e2e-gcp-ovn-upgrade
/test e2e-metal-ipi-ovn-ipv6
/test e2e-vsphere-ovn
/test e2e-vsphere-ovn-upi

@gcs278

gcs278 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

/testwith openshift/origin/main/e2e-gcp-ovn openshift/api#2947

@gcs278

gcs278 commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

infra failures:
/retest

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged. ready-for-human-review Indicates a PR has been reviewed by automated tools and is ready for human review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants