-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 tests for KubeadmControlPlane create workflow #1925
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: detiber 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 |
config/crd/bases/infrastructure.cluster.x-k8s.io_genericclusters.yaml
Outdated
Show resolved
Hide resolved
util/kubeconfig/kubeconfig.go
Outdated
@@ -162,6 +162,9 @@ func GenerateSecretWithOwner(clusterName types.NamespacedName, data []byte, owne | |||
ObjectMeta: metav1.ObjectMeta{ | |||
Name: secret.Name(clusterName.Name, secret.Kubeconfig), | |||
Namespace: clusterName.Namespace, | |||
Labels: map[string]string{ | |||
clusterv1.ClusterLabelName: clusterName.Name, | |||
}, |
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.
Fixes missing cluster-name label on generated kubeconfig secrets
type fakeClientWrapper struct { | ||
fakeClient client.Client | ||
} | ||
|
||
func NewFakeClientWrapperWithScheme(clientScheme *runtime.Scheme, initObjs ...runtime.Object) client.Client { | ||
fakeClient := fake.NewFakeClientWithScheme(clientScheme, initObjs...) | ||
return &fakeClientWrapper{fakeClient: fakeClient} | ||
} | ||
|
||
func (f fakeClientWrapper) Create(ctx context.Context, obj runtime.Object, opts ...client.CreateOption) error { | ||
u, ok := obj.(*unstructured.Unstructured) | ||
if ok { | ||
if u.GetName() == "" && u.GetGenerateName() != "" { | ||
u.SetName(u.GetGenerateName() + util.RandomString(6)) | ||
} | ||
if u.GroupVersionKind().Kind == "GenericMachine" && u.GroupVersionKind().GroupVersion() == externalv1.GroupVersion { | ||
gm := &externalv1.GenericMachine{} | ||
if err := runtime.DefaultUnstructuredConverter.FromUnstructured(u.UnstructuredContent(), gm); err == nil { | ||
return f.fakeClient.Create(ctx, gm, opts...) | ||
} | ||
} | ||
return f.fakeClient.Create(ctx, u, opts...) | ||
} | ||
|
||
return f.fakeClient.Create(ctx, obj, opts...) | ||
} | ||
|
||
func (f fakeClientWrapper) Update(ctx context.Context, obj runtime.Object, opts ...client.UpdateOption) error { | ||
return f.fakeClient.Update(ctx, obj, opts...) | ||
} | ||
|
||
func (f fakeClientWrapper) Delete(ctx context.Context, obj runtime.Object, opts ...client.DeleteOption) error { | ||
return f.fakeClient.Delete(ctx, obj, opts...) | ||
} | ||
|
||
func (f fakeClientWrapper) DeleteAllOf(ctx context.Context, obj runtime.Object, opts ...client.DeleteAllOfOption) error { | ||
return f.fakeClient.DeleteAllOf(ctx, obj, opts...) | ||
} | ||
|
||
func (f fakeClientWrapper) Patch(ctx context.Context, obj runtime.Object, patch client.Patch, opts ...client.PatchOption) error { | ||
return f.fakeClient.Patch(ctx, obj, patch, opts...) | ||
} | ||
|
||
func (f fakeClientWrapper) List(ctx context.Context, list runtime.Object, opts ...client.ListOption) error { | ||
return f.fakeClient.List(ctx, list, opts...) | ||
} | ||
|
||
func (f fakeClientWrapper) Get(ctx context.Context, key client.ObjectKey, obj runtime.Object) error { | ||
return f.fakeClient.Get(ctx, key, obj) | ||
} | ||
|
||
func (f fakeClientWrapper) Status() client.StatusWriter { | ||
return f.fakeClient.Status() | ||
} |
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.
Ugly workaround for fake client not populating Name when GenerateName is used as well as handling of resources created as unstructured and then subsequently accessed as a typed list.
} else { | ||
g.Expect(kubeconfigSecret.Labels).To(gomega.Equal(existingKubeconfigSecret.Labels)) | ||
g.Expect(kubeconfigSecret.Data).To(gomega.Equal(existingKubeconfigSecret.Data)) | ||
g.Expect(kubeconfigSecret.OwnerReferences).NotTo(gomega.ContainElement(*metav1.NewControllerRef(kcp, controlplanev1.GroupVersion.WithKind("KubeadmControlPlane")))) |
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.
do these have sufficient output indicating what fields are problematic without annotations:
g.Expect(...., "some annotation")?
we had such a general problem in k/k/test, but in this case it might not be needed.
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 haven't had much issue with them in the past, mostly because they point directly to the line that caused the error, we can always add annotations later if it starts to become an issue, though.
c50651e
to
34e1212
Compare
9ca98e9
to
bd4c0aa
Compare
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.
comment around test refactor into one test testing one thing (e.g. one TestFunction
for each error condition we want to test)
gist around how to remove the generic resources: https://gist.github.com/chuckha/b01acc137b8b5cd8c0daa28e0bb10636
controlplane/kubeadm/controllers/kubeadm_control_plane_controller_test.go
Outdated
Show resolved
Hide resolved
First breakout PR: #1959 |
f069641
to
0b26064
Compare
Second breakout PR: #1961 |
def6e81
to
71ee133
Compare
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.
Just one comment. This looks great! I'm so happy we were able to avoid the generic machines
71ee133
to
190d38b
Compare
- Add additional generators for kubeadmcontrolplane reconciliation testing
190d38b
to
366530a
Compare
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
amped on the code coverage here, really nice job :)
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 #1756