Use CAPI utils to reduce boilerplate #106
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
|
[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 |
k8s-ci-robot
added
approved
k8s-ci-robot
added
size/M
|
/assign @fabriziopandini @detiber |
| cluster, err := util.GetClusterFromMetadata(ctx, r.Client, machine.ObjectMeta) | ||
| if err != nil { | ||
| if err == util.ErrNoCluster { | ||
| log.Info(fmt.Sprintf("Please set the label %q on the machine equal to the name of the cluster this machine belongs to", capiv1alpha2.MachineClusterLabelName)) |
There was a problem hiding this comment.
is this the right place to log this message?
There was a problem hiding this comment.
I would expect this error to be set from the Machine controller rather than here.
There was a problem hiding this comment.
ok, i'll nix it, thanks
|
/lgtm |
k8s-ci-robot
added
lgtm
k8s-ci-robot
added
size/M
k8s-ci-robot
added
size/L
Signed-off-by: Chuck Ha <[email protected]>
Signed-off-by: Chuck Ha <[email protected]>
There was a problem hiding this comment.
@chuckha thanks for cleaning up the reconcile method!
only one minor from my side, but nothing blocking
| @@ -32,6 +32,42 @@ import ( | |||
| ) | |||
|
|
|||
| func TestControlPlaneInitLockerAcquire(t *testing.T) { | |||
| tests := []struct { | |||
There was a problem hiding this comment.
[1] Are you expecting more test case should be added? otherwise, we can get rid of subtests...
There was a problem hiding this comment.
some context:
I took a test that was testing too many things (error case + non error case) and split it into these two test functions. I kept the structure identical to make sure the tests didn't actually change.
I think it's ok to leave the table structure even with only one test.
My general approach here is to: Write a test that does not use table testing. If I need a second or third case I will add table tests if it makes sense, but overall I prefer the look and readability of table tests.
Are you ok leaving this one as is?
| } | ||
| } | ||
|
|
||
| func TestControlPlaneInitLockerAcquireErrors(t *testing.T) { |
There was a problem hiding this comment.
This one has 3 table tests, I don't think we should get rid of subtests.
There was a problem hiding this comment.
time to take a break...
|
/lgtm |
Signed-off-by: Chuck Ha [email protected]
What this PR does / why we need it:
This PR uses the util code and removes some boilerplate making the Reconcile function shorter and easier to read.
It also bumps CAPI version and refactors some test logic.
Release note: