🌱Add initial Rosa machine pool integration tests #5214
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
needs-priority
needs-rebase
|
Welcome @PanSpagetka! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @PanSpagetka. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
k8s-ci-robot
added
the
size/XL
PanSpagetka
force-pushed
the
rosa-initial-integration-test
branch
from
9e2fe4e
to
2ce8f3d
Compare
k8s-ci-robot
added
cncf-cla: yes
PanSpagetka
force-pushed
the
rosa-initial-integration-test
branch
from
2ce8f3d
to
abce607
Compare
k8s-ci-robot
removed
the
needs-rebase
PanSpagetka
force-pushed
the
rosa-initial-integration-test
branch
3 times, most recently
from
f2c32f1
to
abd21bc
Compare
PanSpagetka
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
@nrb would you add ok-to-test |
main.go
Outdated
| @@ -238,6 +239,8 @@ func main() { | |||
| WatchFilterValue: watchFilterValue, | |||
| WaitInfraPeriod: waitInfraPeriod, | |||
| Endpoints: awsServiceEndpoints, | |||
| NewOCMClient: rosa.NewOCMClient, | |||
There was a problem hiding this comment.
This is not a good idea to create the clients here in the main . why do you need to do that ? for integration test you may just mock the clients.
There was a problem hiding this comment.
No need to expose and init the clients in main. We can init the clients (sts and ocm) internally in the struct and regards the integration test you can mock it similar to this here
There was a problem hiding this comment.
Ok, I moved the initialization to SetupWithManager that seems to me as the only ROSAMachinePoolReconciler method where it fits. I am using same mechanism to create the mock implementation as the STS api.
| @@ -4,7 +4,6 @@ import ( | |||
| "fmt" | |||
|
|
|||
| cmv1 "github.com/openshift-online/ocm-sdk-go/clustersmgmt/v1" | |||
| "github.com/openshift/rosa/pkg/ocm" | |||
There was a problem hiding this comment.
We are using the github.com/openshift/rosa/pkg/ocm in order to avoid duplicate the effort of creating a lib that communicate with OCM. Please keep using it for this first integration test iteration
There was a problem hiding this comment.
We can't use github.com/openshift/rosa/pkg/ocm directly here, because it doesn't expose interface that we could mock. So I had to create mockable interface at our side to be able to mock the ocm calls.
The guys that are developing github.com/openshift/rosa/pkg/ocm are planning to create (and I hope also expose) OCM calls as interface to increase the testability of OCM code. So we could get rid of this in the future. But I am not sure how long will it take them to do it.
| @@ -14,7 +13,7 @@ const ( | |||
|
|
|||
| // CreateAdminUserIfNotExist creates a new admin user withe username/password in the cluster if username doesn't already exist. | |||
| // the user is granted admin privileges by being added to a special IDP called `cluster-admin` which will be created if it doesn't already exist. | |||
| func CreateAdminUserIfNotExist(client *ocm.Client, clusterID, username, password string) error { | |||
There was a problem hiding this comment.
We are using the github.com/openshift/rosa/pkg/ocm in order to avoid duplicate the effort of creating a lib that communicate with OCM. Please keep using it for this first integration test iteration
There was a problem hiding this comment.
okay, just github duplication
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
PanSpagetka
force-pushed
the
rosa-initial-integration-test
branch
2 times, most recently
from
5eeb8ef
to
2cbbf46
Compare
main.go
Outdated
| @@ -238,6 +239,8 @@ func main() { | |||
| WatchFilterValue: watchFilterValue, | |||
| WaitInfraPeriod: waitInfraPeriod, | |||
| Endpoints: awsServiceEndpoints, | |||
| NewOCMClient: rosa.NewOCMClient, | |||
There was a problem hiding this comment.
No need to expose and init the clients in main. We can init the clients (sts and ocm) internally in the struct and regards the integration test you can mock it similar to this here
pkg/rosa/client.go
Outdated
| token, url, err := ocmCredentials(ctx, rosaScope) | ||
| if err != nil { | ||
| return nil, err | ||
| return ocmclient{}, err |
pkg/rosa/client.go
Outdated
| } | ||
|
|
||
| // NewMockOCMClient creates a new empty ocm.Client without any real connection. | ||
| func NewMockOCMClient(ctx context.Context, rosaScope *scope.ROSAControlPlaneScope) (OCMClient, error) { |
There was a problem hiding this comment.
no need for function params. in fact it not a function just variable
c := ocmclient{ ocmClient: ocm.Client{}, }
2cbbf46
to
cdc81e8
Compare
|
/test pull-cluster-api-provider-aws-test |
| @@ -203,7 +211,7 @@ func (r *ROSAControlPlaneReconciler) reconcileNormal(ctx context.Context, rosaSc | |||
| } | |||
| } | |||
|
|
|||
| ocmClient, err := rosa.NewOCMClient(ctx, rosaScope) | |||
| ocmClient, err := r.NewOCMClient(ctx, rosaScope) | |||
There was a problem hiding this comment.
check for nil, r.NewOCMClient could be nil
| @@ -186,7 +194,7 @@ func (r *ROSAMachinePoolReconciler) reconcileNormal(ctx context.Context, | |||
| } | |||
| } | |||
|
|
|||
| ocmClient, err := rosa.NewOCMClient(ctx, rosaControlPlaneScope) | |||
| ocmClient, err := r.NewOCMClient(ctx, rosaControlPlaneScope) | |||
| Template: clusterv1.MachineTemplateSpec{ | ||
| Spec: clusterv1.MachineSpec{ | ||
| ClusterName: ownerCluster.Name, | ||
| }, |
There was a problem hiding this comment.
We should add the infrastructureRef pointing to RosaMachinePool to have proper test,
| Kind: "ROSAMachinePool", | ||
| APIVersion: expinfrav1.GroupVersion.String(), | ||
| }, | ||
| Spec: expinfrav1.RosaMachinePoolSpec{}, |
There was a problem hiding this comment.
we should at least set the version, otherwise it should fail
|
|
||
| result, err := r.Reconcile(ctx, req) | ||
|
|
||
| g.Expect(err).ToNot(HaveOccurred()) |
There was a problem hiding this comment.
Please add more validations, check for the RosaMachinePool.status
| defer teardown() | ||
|
|
||
| deleteTime := metav1.NewTime(time.Now().Add(5 * time.Second)) | ||
| rosaMachinePool.ObjectMeta.Finalizers = []string{"finalizer-rosa"} |
There was a problem hiding this comment.
Not sure what are you testing here, but finalizer should be added by the RosaMachinePool controller
|
|
||
| result, err := r.Reconcile(ctx, req) | ||
| g.Expect(err).ToNot(HaveOccurred()) | ||
| g.Expect(result).To(Equal(ctrl.Result{})) |
There was a problem hiding this comment.
You need to validate if the RosaMachinePool deleted.
pkg/rosa/client.go
Outdated
| @@ -20,16 +22,142 @@ const ( | |||
| ocmAPIURLKey = "ocmApiUrl" | |||
| ) | |||
|
|
|||
| type ocmclient struct { | |||
There was a problem hiding this comment.
OCMClient not ocmclient and please move the OCMClient struct, interface and funcs to another file ex; ocmclient.go
| @@ -14,7 +13,7 @@ const ( | |||
|
|
|||
| // CreateAdminUserIfNotExist creates a new admin user withe username/password in the cluster if username doesn't already exist. | |||
| // the user is granted admin privileges by being added to a special IDP called `cluster-admin` which will be created if it doesn't already exist. | |||
| func CreateAdminUserIfNotExist(client *ocm.Client, clusterID, username, password string) error { | |||
There was a problem hiding this comment.
okay, just github duplication
PanSpagetka
force-pushed
the
rosa-initial-integration-test
branch
from
cdc81e8
to
23e5a17
Compare
|
@PanSpagetka: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
k8s-ci-robot
added
the
do-not-merge/work-in-progress
PanSpagetka
force-pushed
the
rosa-initial-integration-test
branch
from
23e5a17
to
975b9c4
Compare
What type of PR is this?
Adding tests. I am not sure what kind should apply so I am not applying any.,
What this PR does / why we need it:
Adding basic integration tests for
ROSAMachinePoolReconciler. One test case for creating new machine pool and for deleting.To be able to mock OCM and STS calls I had to do small refactoring.
Checklist:
Release note: