-
Notifications
You must be signed in to change notification settings - Fork 531
baremetal: enhancement for optional provisioning network #386
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
Conversation
This enhancement describes what's required to make the provisioning network optional.
| the external network that are used for the provisioning services. | ||
|
|
||
| The same field will be added to the Provisioning CRD, with the | ||
| provisioningDHCPExternal field removed. |
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.
Removing the existing field will take a couple of releases. In the mean time, we will need to declare which field takes precedence over the other and support not having the ProvisioningNetwork option set at all.
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.
What is the benefit of removing it? Is it worth breaking backwards compatibility instead of just dropping it from docs?
Can you also add a reference to the current definition of this CRD?
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.
Hm ok, I'll update this section. We could leave it around forever I suppose, but undocumented.
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.
Note that the Provisioning CRD was added purely to enable the installer to pass data to the MAO ref openshift/machine-api-operator#460 - we had a bunch of discussion on openshift/api#540 before that, and one of the reasons it wasn't added to openshift/api was avoid publishing any "external" interface.
So IMO it's probably fine to deprecate then later remove the existing option, given that it's highly likely that the only user of this interface is the installer?
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.
ACK thanks for clarifying
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.
I clarified this, please have a look to see if it's clear.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dhellmann, stbenjam 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 |
This enhancement describes what's required to make the provisioning
network optional. This enhancement adds a new option to the installer (
provisioningNetwork) that takes the valuesmanaged,unmanaged, anddisabled. The formerprovisioningDHCPExternaloption is deprecated.