-
Notifications
You must be signed in to change notification settings - Fork 1.5k
OCPBUGS-881: fail to create install-config.yaml as apiVIP and ingress VIP are not in machine networks #6469
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
OCPBUGS-881: fail to create install-config.yaml as apiVIP and ingress VIP are not in machine networks #6469
Conversation
|
Skipping CI for Draft Pull Request. |
|
@rvanderp3: This pull request references Jira Issue OCPBUGS-881, 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 kubernetes/test-infra repository. |
|
/test e2e-vsphere |
hmmm, these tests don't really test the behavior since these are install-config survey changes. We'll run them anyway though. |
|
@rvanderp3: This pull request references Jira Issue OCPBUGS-881, 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 kubernetes/test-infra repository. |
|
cc: @patrickdillon |
|
/lgtm |
patrickdillon
left a comment
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.
/approve
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.
nit: It might be simpler/less confusing to export defaultMachineCIDR from package types/defaults; because I wonder if this implies "we're creating the install config here", which we're 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.
yeah, I was torn on that tbh. My concern was that based on the platform, that defaultMachineCIDR might not be the actual defaultMachineCIDR. While this doesn't impact any current onprem platforms, I was concerned this might cause some confusion down the line. another option here might be to pull the logic out of SetInstallConfigDefaults that determines the final defaultMachineCIDR of the platform.
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.
thinking about this a little more, I think i'll go ahead and revert to your suggestion here.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon 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 |
|
/retest-required Remaining retests: 0 against base HEAD 7b739cd and 2 for PR HEAD d6f29ad6ba1cf7bdea2b2b35e21574bdaa5c439e in total |
|
/retest-required Remaining retests: 0 against base HEAD bb20681 and 1 for PR HEAD d6f29ad6ba1cf7bdea2b2b35e21574bdaa5c439e in total |
|
/hold |
…VIP are not in machine networks validate VIPs are in machineNetwork in survey
|
/hold cancel |
|
/override ci/prow/e2e-vsphere-ovn This is just failing on a flake unrelated to this PR. |
|
@patrickdillon: Overrode contexts on behalf of patrickdillon: ci/prow/e2e-vsphere-ovn 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 kubernetes/test-infra repository. |
|
hmmmm ... something has changed. taking a look. |
504da80 to
1314617
Compare
|
looks like linting rules changed. ive pushed an update. |
1314617 to
532bd4c
Compare
Correct. We've standardized the import order [1] and replaced the deprecated linter by a new one. |
|
/lgtm |
|
/hold Revision 532bd4c was retested 3 times: holding |
|
/hold cancel |
|
@rvanderp3: The following tests 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/test-infra repository. I understand the commands that are listed here. |
|
@rvanderp3: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-881 has been moved to the MODIFIED state. 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 kubernetes/test-infra repository. |
|
/cherrypick release-4.12 |
|
@rvanderp3: new pull request created: #6783 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 kubernetes/test-infra repository. |
validate VIPs are in machineNetwork in survey. validation was added for vsphere, ovirt, and nutanix.