Add MachinePhase type and helper functions #1080
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vincepri 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 |
k8s-ci-robot
added
the
approved
|
/assign @ncdc |
k8s-ci-robot
added
the
size/M
vincepri
force-pushed
the
add-typed-phasex
branch
from
86300fb to
ee9b579
Compare
|
/assign @detiber |
vincepri
force-pushed
the
add-typed-phasex
branch
from
ee9b579 to
52f929e
Compare
k8s-ci-robot
added
size/L
|
@detiber ptal, I think this should be good to go |
| } | ||
|
|
||
| // SetTypedPhase sets the Phase field to the string representation of MachinePhase. | ||
| func (m *MachineStatus) SetTypedPhase(p MachinePhase) { |
There was a problem hiding this comment.
I can change it, thinking was that Phase is already a field and I wanted to avoid confusion for users of these methods, wdyt?
There was a problem hiding this comment.
I thought we agreed that Phase would remain a string: #997 (comment)
There was a problem hiding this comment.
Since the commits were squashed I can't see the history. Either way, the current proposal makes the state machine explicit so I think it's fair to type it. I'll open an issue suggesting changes to the proposal out-of-band with this PR.
There was a problem hiding this comment.
Yeah, it's unchanged from v1alpha1, these methods are helpers to deal with the MachinePhase type (string)
vincepri
force-pushed
the
add-typed-phasex
branch
2 times, most recently
from
fc8f51f to
444f83a
Compare
|
@detiber ptal |
| @@ -711,6 +712,9 @@ spec: | |||
| up status from the Node to the Machine. It is entirely optional, but | |||
| useful for end-user UX if it’s present. | |||
| type: string | |||
| required: | |||
There was a problem hiding this comment.
These are still showing as required.
There was a problem hiding this comment.
Hmm. I did set them as +optional did I miss something else?
There was a problem hiding this comment.
Maybe it should be a pointer with omitempty?
/cc @ncdc
There was a problem hiding this comment.
I don't think so, does the crd definition need to be regenerated?
There was a problem hiding this comment.
Pointer should only be needed if you need to differentiate unset from false.
There was a problem hiding this comment.
Pointer is required for optional fields
There was a problem hiding this comment.
There was a problem hiding this comment.
Tried different combinations, and it seems I need omitempty to be actually optional
There was a problem hiding this comment.
so pointer and omitempty it is
Signed-off-by: Vince Prignano <vincepri@vmware.com>
vincepri
force-pushed
the
add-typed-phasex
branch
from
444f83a to
3003102
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/test pull-cluster-api-test |
Signed-off-by: Vince Prignano vincepri@vmware.com
What this PR does / why we need it:
This PR adds information to v1alpha2 types about Machine Phases as described in the bootstrap proposal.
In addition, it adds some helper methods on MachineStatus to ease the interaction with the Phase (*string) field.
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 #1036
Special notes for your reviewer:
Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.
Release note: