-
Notifications
You must be signed in to change notification settings - Fork 585
Add optional new fields to NutanixMachineProviderConfig #1390
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
Add optional new fields to NutanixMachineProviderConfig #1390
Conversation
|
Hello @yanhua121! Some important instructions when contributing to openshift/api: For merging purposes, this repository follows the no-Feature-Freeze process which means that in addition to the standard
OR
Who should apply these qe/docs/px labels?
|
a93873f to
4416e0c
Compare
4416e0c to
f054742
Compare
|
/lgtm |
|
the |
|
the |
f054742 to
17ffc12
Compare
|
The Machine webhook validation for these new fields can be tested with openshift/machine-api-operator#1117. |
|
/label qe-approved |
JoelSpeed
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.
Quick question, do we need the omitempty on these fields? If we remove it, the fields will show in a serialized version with empty values, which then hints to users "oh, that's an option I can use", so it improves the discoverability, which is generally something we advise for configuration APIs such as these
@JoelSpeed The only concern I have is if we remove the 'omitempty' on these optional fields, the Machine/MachineSet yaml used in a 4.13 cluster then cannot use to a OCP cluster with earlier version 4.11/4.12. In some scenario, customer may have clusters with different OCP versions. They may want to deploy the Machine/MachineSet yamls to all the cluster. |
|
Unknown fields are just dropped on older versions so it would be ok to have the new fields with empty values and be applied to the older versions. I don't think changing omitempty changes that scenario really. We can add it back if we find issues but I think for now it should be removed for discoverability |
@JoelSpeed Okay, then I will go ahead to remove 'omitempty' for these optional fields. |
17ffc12 to
e3fa411
Compare
e3fa411 to
bf73e5c
Compare
|
/lgtm |
|
/retest-required |
bf73e5c to
c7a709d
Compare
|
@yanhua121: 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/test-infra repository. I understand the commands that are listed here. |
elmiko
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.
/lgtm
|
/px-approved |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: elmiko, JoelSpeed, yanhua121 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 |
|
/label px-approved |
Add some optional new fields to NutanixMachineProviderConfig, for Windows support, etc.