-
Notifications
You must be signed in to change notification settings - Fork 1.9k
modules/installation-bootstrap-gather: --master for each machine #17196
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
modules/installation-bootstrap-gather: --master for each machine #17196
Conversation
|
Or maybe it works accidentally now because we have poor quoting/validation ;). |
|
Pushed an additional commit to catch up with openshift/installer#1822, which should be backported to 4.2, but not to 4.1. |
kalexand-rh
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.
Thank you for the update! I might change it up a bit.
78a51d9 to
ecbb267
Compare
|
Squashed in your suggestions. |
ecbb267 to
9f006d0
Compare
Because of the different backport depths, I just spun the second commit off into its own PR (#17651). This one can go back to 4.1 and 4.2. |
|
@shlao, will you PTAL? |
|
@ahardin-rh it's good, but In big Corps, there are, maybe, more than 3 masters. |
I think that that would make the command harder to use, but we can be more explicit in the note. @wking, what do you think of the commit that I added? |
0b1e989 to
518234b
Compare
|
I've pushed 0b1e9890c -> 518234b7e, softening @kalexand-rh's wording a bit to drop the "must" and allowing for clusters with fewer control plane machines (we may not support one or two control plane machines, but the gather logic doesn't care about that). |
This is what we do in CI since openshift/release@3013859bdc (ci-operator/templates/openshift/installer/cluster-launch-installer-upi-e2e: Gather on bootstrap failure, 2019-08-13, openshift/release#4734), so we know it works. Comma-delimited machines won't work, as described in that release commit. I'd be surprised if space-delimited worked.
8a32a09 to
1e49f68
Compare
|
I think this will work. Thank you! |
|
/cherrypick enterprise-4.2 |
|
/cherrypick enterprise-4.3 |
|
@kalexand-rh: new pull request created: #18022 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. |
|
@kalexand-rh: new pull request created: #18023 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. |
This is what we do in CI since openshift/release@3013859bdc (openshift/release#4734), so we know it works. Comma-delimited machines won't work, as described in that release commit. I'd be surprised if space-delimited worked.