-
Notifications
You must be signed in to change notification settings - Fork 562
CORS-4155: Add Feature gate and update Infrastructure CR for Azure Cluster Hosted DNS #2404
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CORS-4155: Add Feature gate and update Infrastructure CR for Azure Cluster Hosted DNS #2404
Conversation
Hello @sadasu! Some important instructions when contributing to openshift/api: |
49fea39
to
ab30eb7
Compare
/retest-required |
1 similar comment
/retest-required |
@sadasu: This pull request references CORS-4155 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.20.0" version, but no target version was set. In 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside from removing the +nullable
marker, the API changes looks good to me
onCreate: | ||
- name: Should be able to create a minimal Infrastructure | ||
initial: | | ||
apiVersion: config.openshift.io/v1 | ||
kind: Infrastructure | ||
spec: {} # No spec is required for a Infrastructure | ||
expected: | | ||
apiVersion: config.openshift.io/v1 | ||
kind: Infrastructure | ||
spec: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the CI failure, this looks like an invalid test case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI failures are actually talking about ControllerConfig
which is different, something has broken them though, @sandhya PTAL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+nullable
marker has been removed. Infra CR is embedded within ControllerConfig
. I'll take a look at the latest CI failures as soon as they become available.
fc8afd6
to
409f2a7
Compare
13a30cb
to
14134b3
Compare
/retest-required |
@everettraven and @JoelSpeed Could PTAL? Thanks! |
/retest-required |
@@ -336,6 +336,14 @@ var ( | |||
enableIn(configv1.DevPreviewNoUpgrade, configv1.TechPreviewNoUpgrade). | |||
mustRegister() | |||
|
|||
FeatureGateAzureClusterHostedDNSInstall = newFeatureGate("AzureClusterHostedDNSInstall"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does this one need the *Install
suffix but the other platform gates did not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/openshift/api/blob/master/tools/codegen/cmd/featuregate-test-analyzer.go#L566 requires names of Installer feature gates to contain "Install".
#2423 is starting the work of renaming existing featuregates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides a small question on the naming of the gate compared to previous ones, this LGTM.
@JoelSpeed feel free to take a look
/lgtm |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: everettraven, JoelSpeed, sadasu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@sadasu: all tests passed! Full PR test history. Your PR dashboard. 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 kubernetes-sigs/prow repository. I understand the commands that are listed here. |
[ART PR BUILD NOTIFIER] Distgit: ose-cluster-config-api |
Identical features were added to GCP and AWS platforms and is now being extended to Azure too.