OCPBUGS-74511: Remove RouteExternalCertificate feature gate - #2962
OCPBUGS-74511: Remove RouteExternalCertificate feature gate#2962jcmoraisjr wants to merge 1 commit into
Conversation
RouteExternalCertificate is enabled by default, this update is removing its declaration from the legacy featuregates list, from CRD declarations and from type annotations. https://redhat.atlassian.net/browse/OCPBUGS-74511
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@jcmoraisjr: This pull request references Jira Issue OCPBUGS-74511, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
Hello @jcmoraisjr! Some important instructions when contributing to openshift/api: |
|
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: Pro Plus Run ID: ⛔ Files ignored due to path filters (4)
📒 Files selected for processing (15)
💤 Files with no reviewable changes (13)
📝 WalkthroughWalkthroughThe RouteExternalCertificate feature gate was removed from feature documentation, global and legacy registrations, and 4.10 payload manifests. TLSConfig externalCertificate is no longer guarded by that gate, while mutual exclusivity with certificate is enforced unconditionally. The related route test no longer configures the removed feature gate. 🚥 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 |
PR Summary by QodoRemove RouteExternalCertificate feature gate and ungate Route externalCertificate
AI Description
Diagram
High-Level Assessment
Files changed (18)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
29 rules✅ Skills:
api-review 1. Undocumented TLSConfig XValidation rule
|
| // TLSConfig defines config used to secure a route and provide termination | ||
| // | ||
| // +kubebuilder:validation:XValidation:rule="has(self.termination) && has(self.insecureEdgeTerminationPolicy) ? !((self.termination=='passthrough') && (self.insecureEdgeTerminationPolicy=='Allow')) : true", message="cannot have both spec.tls.termination: passthrough and spec.tls.insecureEdgeTerminationPolicy: Allow" | ||
| // +openshift:validation:FeatureGateAwareXValidation:featureGate=RouteExternalCertificate,rule="!(has(self.certificate) && has(self.externalCertificate))", message="cannot have both spec.tls.certificate and spec.tls.externalCertificate" | ||
| // +kubebuilder:validation:XValidation:rule="!(has(self.certificate) && has(self.externalCertificate))", message="cannot have both spec.tls.certificate and spec.tls.externalCertificate" |
There was a problem hiding this comment.
1. Undocumented tlsconfig xvalidation rule 📘 Rule violation ⚙ Maintainability
TLSConfig has a new +kubebuilder:validation:XValidation rule without a plain-language Go comment explaining the cross-field constraint. This reduces API clarity and violates the requirement to document XValidation rules in comments.
Agent Prompt
## Issue description
`TLSConfig` includes a `+kubebuilder:validation:XValidation` marker, but the surrounding Go doc comment does not explain (in plain language) what the rule enforces.
## Issue Context
The compliance requirement expects a human-readable explanation immediately above the XValidation marker so readers understand the cross-field constraint without parsing CEL/annotation syntax.
## Fix Focus Areas
- route/v1/types.go[422-425]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| featureGates: | ||
| - RouteExternalCertificate |
There was a problem hiding this comment.
2. Stale test gating docs 🐞 Bug ⚙ Maintainability
The PR removes the featureGates requirement from the RouteExternalCertificate test suite YAML, making that suite run as ungated, but README.md still presents it as an example of a feature-gated test for RouteExternalCertificate. This mismatch can mislead contributors about how to author/associate feature-gated API validation tests.
Agent Prompt
## Issue description
`route/v1/tests/routes.route.openshift.io/RouteExternalCertificate.yaml` is no longer feature-gated (the `featureGates` field was removed), but README.md still documents it as a feature-gated example for `RouteExternalCertificate`.
## Issue Context
The test runner treats an empty/missing `featureGates` list as an ungated suite and runs it against all matching CRD manifests.
## Fix Focus Areas
- README.md[55-82]
- route/v1/tests/routes.route.openshift.io/RouteExternalCertificate.yaml[1-6]
## Suggested fix
- Update README.md to remove/replace the `RouteExternalCertificate.yaml` “feature-gated example” section (since the gate was removed), or rewrite it to reflect the new ungated state.
- (Optional but clearer) Rename `RouteExternalCertificate.yaml` to a non-featuregate-specific filename (keeping it under the same tests directory), since it now contains ungated validation tests.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
/lgtm |
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JoelSpeed 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 |
|
based on RouteExternalCertificate, i am marking this as verified |
|
@melvinjoseph86: 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. |
|
/retest-required |
|
@jcmoraisjr: The following test 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. |
RouteExternalCertificate is enabled by default, this update is removing its declaration from the legacy featuregates list, from CRD declarations and from type annotations.
This PR is reapplying #2693, which was reverted by #2862 due to the missing to bump o/kubernetes in o/origin. This bump was done via openshift/origin#31237.
https://redhat.atlassian.net/browse/OCPBUGS-74511