OCPBUGS-74511: remove RouteExternalCertificate feature gate#1355
Conversation
|
@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. |
|
/retest |
|
@jcmoraisjr i think we need a API PR also to test.. |
|
@jcmoraisjr: This pull request references Jira Issue OCPBUGS-74511, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
RouteExternalCertificate is enabled by default, this is the second half of the changes to remove it and should be merged only after openshift/cluster-ingress-operator#1355 being merged.
cc2729b to
9a9972e
Compare
|
/retest |
9a9972e to
d50f599
Compare
| ) | ||
| } | ||
| env = append(env, | ||
| corev1.EnvVar{Name: "ROUTER_ENABLE_EXTERNAL_CERTIFICATE", Value: "true"}, |
There was a problem hiding this comment.
I was wondering what if we completely drop this environment variable, now that it's default value is true
The only consumer of this environment is here: https://github.com/openshift/router/blob/d2db065ae452ecfdb482f0ac4c6778b0c0e48b7f/pkg/cmd/infra/router/router.go#L115
So, should we remove the logic from o/router first, then here?
/cc @Miciah @alebedev87
There was a problem hiding this comment.
Router change in case this is the way to go: openshift/router#730
There was a problem hiding this comment.
I was wondering what if we completely drop this environment variable
After an internal discussion we agreed on keeping the existing envvar as the safest way forward.
|
Verified using cluster bot The Hence marking 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 |
|
/assign |
| t.Fatalf("invalid router Deployment: %v", err) | ||
| } | ||
|
|
||
| // Verify that router external certificate env var is set to true. |
There was a problem hiding this comment.
Now as we set this envvar unconditionally, this comment is the accurate one.
There was a problem hiding this comment.
Ow what a mess I did. Just addressed.
RouteExternalCertificate is now enabled by default. This update is removing references to this feature gate, hardcoding its behavior as enabled. https://issues.redhat.com/browse/OCPBUGS-74511
d50f599 to
70bab84
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe changes remove the RouteExternalCertificateEnabled configuration field from the ingress controller setup. The ROUTER_ENABLE_EXTERNAL_CERTIFICATE environment variable is now unconditionally set to "true" in deployments, replacing the previous conditional logic that gated its inclusion. Feature flag computation for FeatureGateRouteExternalCertificate is removed from the operator initialization. Test coverage is simplified to validate only the scenario where external certificate support is enabled. These modifications consolidate the external certificate feature into a permanent, always-enabled state. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alebedev87 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 |
|
reapplying verified label based on #1355 (comment) |
|
@melvinjoseph86: This PR has been marked to be verified later 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 |
|
/test e2e-gcp-operator |
|
@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. |
|
@jcmoraisjr: Jira Issue OCPBUGS-74511: Some pull requests linked via external trackers have merged: The following pull request, linked via external tracker, has not merged:
All associated pull requests must be merged or unlinked from the Jira bug in order for it to move to the next state. Once unlinked, request a bug refresh with Jira Issue OCPBUGS-74511 has not been moved to the MODIFIED state. This PR is marked as verified-later. Jira issue(s) in the title of this PR will require post-merge verification. After testing, it must be manually moved to the 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. |
RouteExternalCertificate is enabled by default, this is the first half of the change to remove it. After merging, its declaration should be removed from openshift/api as well.
Jira: https://issues.redhat.com/browse/OCPBUGS-74511