-
Notifications
You must be signed in to change notification settings - Fork 540
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
馃弮 Update Machine FailureDomain field #1507
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chuckha 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 |
actually... i'm just realizing this is defined as /hold |
8d0385c
to
c939f68
Compare
|
||
// AvailabilityZone is references the AWS availability zone to use for this instance. | ||
// If multiple subnets are matched for the availability zone, the first one return is picked. | ||
// | ||
// DEPRECATED: Switch to FailureDomainID. | ||
// DEPRECATED: Switch to FailureDomain. |
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.
Not directly related to this PR, but since we are deprecating this field, should we be handling some type of automatic migration to the FailureDomain field as part of the conversion webhook? It looks like we are doing the reverse (populating the AvailabilityZone in v1alpha2 if FailureDomain is set in v1alpha3).
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 we can, we might want to remove the field altogether though, this is only marked as a deprecation and we have a fallback in place that looks at the AvailabilityZone
field if FailureDomain is nil.
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'm good with removing the field altogether as well :)
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'll file an issue so we don't forget as this is out of scope for the original intent of this PR
Signed-off-by: Chuck Ha <chuckh@vmware.com>
c939f68
to
2e24088
Compare
/hold cancel As stated above, held because I wanted to swap directions the PR was going. That is done. |
/lgtm |
Signed-off-by: Chuck Ha chuckh@vmware.com
Make the field name consistent with the json tag name and other documentation.
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):related to kubernetes-sigs/cluster-api#2185