NE-2217: Add haproxyVersion in IngressController API#2895
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
Hello @jcmoraisjr! Some important instructions when contributing to openshift/api: |
|
@jcmoraisjr: This pull request references NE-2217 which is a valid 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. |
|
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 YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: ⛔ Files ignored due to path filters (7)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughIngressController now includes a feature-gated 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 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": tools/_output/bin/kube-api-linter.so, plugin: not implemented Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@operator/v1/types_ingresscontroller.go`:
- Around line 2258-2271: The EffectiveHAProxyOCPVersion field is marked as
optional but its comment does not document the expected behavior when the field
is omitted. Update the comment for the EffectiveHAProxyOCPVersion field to
explicitly specify what it means when this status field is absent, such as
whether omission indicates an unresolved value, feature-gate-disabled state, or
another condition. This clarification is required as per coding guidelines for
all optional fields.
- Around line 2341-2346: The OCPVersion type has kubebuilder validation markers
for MinLength=3 and MaxLength=8, but the comment only documents the format
requirement without mentioning the length constraints. Update the comment for
the OCPVersion type to include explicit documentation of the length constraints
(minimum 3 characters, maximum 8 characters) in human-readable terms alongside
the existing format documentation to match the validation markers and follow
coding guidelines.
🪄 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: 302ef052-c626-4396-a8ab-1df4406a6900
⛔ Files ignored due to path filters (5)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/IngressControllerMultipleHAProxyVersions.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (1)
operator/v1/types_ingresscontroller.go
2c6e76b to
6d7c1c8
Compare
|
@yuqi-zhang here is the API update for the openshift/enhancements#1965 you also reviewed. |
6d7c1c8 to
d75078d
Compare
d75078d to
d41e8c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerMultipleHAProxyVersions.yaml (1)
86-185: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMissing onUpdate negative test for invalid
haproxyVersion.
onCreatetests reject empty ("") and unsupported ("2.6") values, butonUpdateonly covers valid transitions (unset↔2.8, 2.8→3.2). Add an update case setting an unsupported/empty value to confirm the enum validation is enforced symmetrically on update.➕ Suggested additional test case
- name: Should not be able to update to an unsupported version initial: | apiVersion: operator.openshift.io/v1 kind: IngressController metadata: name: default namespace: openshift-ingress-operator spec: httpEmptyRequestsPolicy: Respond idleConnectionTerminationPolicy: Immediate closedClientConnectionPolicy: Continue haproxyVersion: "2.8" updated: | apiVersion: operator.openshift.io/v1 kind: IngressController metadata: name: default namespace: openshift-ingress-operator spec: httpEmptyRequestsPolicy: Respond idleConnectionTerminationPolicy: Immediate closedClientConnectionPolicy: Continue haproxyVersion: "2.6" expectedError: 'Unsupported value: "2.6": supported values: "2.8", "3.2"'🤖 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 `@operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerMultipleHAProxyVersions.yaml` around lines 86 - 185, The onUpdate coverage in IngressControllerMultipleHAProxyVersions is missing a negative validation case for haproxyVersion, so add an update test that changes a valid existing value to an unsupported or empty value and expects the enum rejection. Use the existing onUpdate scenarios in IngressControllerMultipleHAProxyVersions and mirror the validation style already used in onCreate so the update path is checked symmetrically for haproxyVersion.
🤖 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 `@operator/v1/types_ingresscontroller.go`:
- Around line 389-413: The HAProxyVersion field comment repeats the same phrase
twice in the upgrade-preflight sentence, so tighten the wording to remove the
duplicate “block the cluster upgrade” phrasing. Update the comment near
HAProxyVersion in types_ingresscontroller.go to say the preflight check blocks
the upgrade until the field is updated, keeping the rest of the migration
guidance intact and concise.
---
Nitpick comments:
In
`@operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerMultipleHAProxyVersions.yaml`:
- Around line 86-185: The onUpdate coverage in
IngressControllerMultipleHAProxyVersions is missing a negative validation case
for haproxyVersion, so add an update test that changes a valid existing value to
an unsupported or empty value and expects the enum rejection. Use the existing
onUpdate scenarios in IngressControllerMultipleHAProxyVersions and mirror the
validation style already used in onCreate so the update path is checked
symmetrically for haproxyVersion.
🪄 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: 23c20d00-d012-4aff-8d68-022b90037948
⛔ Files ignored due to path filters (7)
openapi/generated_openapi/zz_generated.openapi.gois excluded by!openapi/**,!**/zz_generated*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-CustomNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-DevPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.crd-manifests/0000_50_ingress_00_ingresscontrollers-TechPreviewNoUpgrade.crd.yamlis excluded by!**/zz_generated.crd-manifests/*operator/v1/zz_generated.featuregated-crd-manifests.yamlis excluded by!**/zz_generated*operator/v1/zz_generated.featuregated-crd-manifests/ingresscontrollers.operator.openshift.io/IngressControllerMultipleHAProxyVersions.yamlis excluded by!**/zz_generated.featuregated-crd-manifests/**operator/v1/zz_generated.swagger_doc_generated.gois excluded by!**/zz_generated*
📒 Files selected for processing (2)
operator/v1/tests/ingresscontrollers.operator.openshift.io/IngressControllerMultipleHAProxyVersions.yamloperator/v1/types_ingresscontroller.go
|
@jcmoraisjr: 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. |
|
I've reviewed this as a part of the EP already and it looks good to me. The docs are quite specific and will go stale fast, but I guess that doesn't matter because the enum values will go stale in the next release anyways. So we are forced to update the API every release (maybe we can automate it later). I think we are waiting on @yuqi-zhang to follow up on this one. /lgtm |
|
Pipeline controller notification No second-stage tests were triggered for this PR. This can happen when:
Use |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: gcs278 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Add haproxyVersion field in IngressController API. This field adds the ability to revert HAProxy version to a previous one, as well as pin the current HAProxy version during OCP upgrades. EP: openshift/enhancements#1965 Jira: https://redhat.atlassian.net/browse/NE-2217
d41e8c5 to
bdd334a
Compare
|
New changes are detected. LGTM label has been removed. |
Add haproxyVersion field in IngressController API. This field adds the ability to revert HAProxy version to a previous one, as well as pin the current HAProxy version during OCP upgrades.
EP: openshift/enhancements#1965
Jira: https://redhat.atlassian.net/browse/NE-2217