Add MachineRole to the GCP-specific provider conifg #400
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: roberthbailey 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
|
/hold I need to run at least one actual deployment test with clusterctl before we merge this. |
k8s-ci-robot
added
the
do-not-merge/hold
cloud/google/machineactuator.go
Outdated
| currentConfig, err := gce.machineproviderconfig(currentMachine.Spec.ProviderConfig) | ||
| if err != nil { | ||
| return gce.handleMachineError(currentMachine, apierrors.InvalidMachineConfiguration( | ||
| "Cannot unmarshal machine's providerConfig field: %v", err), createEventAction) |
There was a problem hiding this comment.
We are in the Update function so createEventAction seems wrong here (note that on line 417 noEventAction is used.
cloud/google/machineactuator.go
Outdated
| } | ||
| } | ||
| return false | ||
|
|
There was a problem hiding this comment.
fixed (would have thought gofmt would take care of that one...)
| @@ -10,6 +10,8 @@ items: | |||
| value: | |||
| apiVersion: "gceproviderconfig/v1alpha1" | |||
| kind: "GCEMachineProviderConfig" | |||
| roles: | |||
| - Master | |||
There was a problem hiding this comment.
It seems like there should be a corresponding deletion on lines 25 and 26
There was a problem hiding this comment.
some of the common code still uses the roles in the api proper; i'll delete those ones once i fix the common code to no longer depend on roles.
| @@ -33,6 +35,8 @@ items: | |||
| value: | |||
| apiVersion: "gceproviderconfig/v1alpha1" | |||
| kind: "GCEMachineProviderConfig" | |||
| roles: | |||
| - Node | |||
There was a problem hiding this comment.
It seems like there should be a corresponding deletion on lines 49 and 50
There was a problem hiding this comment.
same as above.
cloud/google/machineactuator.go
Outdated
| "Cannot unmarshal machine's providerConfig field: %v", err), createEventAction) | ||
| } | ||
|
|
||
|
|
roberthbailey
force-pushed
the
gcp-machine-role
branch
from
818b74e
to
b1d62c1
Compare
|
I verified that I can still create a cluster with clusterctl, and addressed review feedback. /hold cancel |
roberthbailey
removed
the
do-not-merge/hold
|
lgtm |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
What this PR does / why we need it: Begins demoting the notion of machine role down into the GCP specific provider config (since that is the primary place it is currently used in the code base).
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):First PR towards fixing #338
Special notes for your reviewer:
Release note:
@kubernetes/kube-deploy-reviewers