🌱Add initial Rosa machine pool integration tests #5214
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
|
|
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.
pkg/rosa/idps.go
Outdated
| @@ -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.
pkg/rosa/idps.go
Outdated
| @@ -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
from
abd21bc to
5eeb8ef
Compare
k8s-ci-robot
added
the
lgtm
|
/area testing |
PanSpagetka
force-pushed
the
rosa-initial-integration-test
branch
from
e027cf5 to
76a2803
Compare
k8s-ci-robot
removed
the
lgtm
PanSpagetka
force-pushed
the
rosa-initial-integration-test
branch
from
76a2803 to
29c0505
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: nrb 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
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/retest |
1 similar comment
|
/retest |
|
/test pull-cluster-api-provider-aws-test Max number of VPCs reached |
|
/test pull-cluster-api-provider-aws-test |
|
/retest |
|
/test pull-cluster-api-provider-aws-test |
|
/retest |
|
/retest VPC capacity issues, not flakes in the suite. |
|
/test pull-cluster-api-provider-aws-test |
PanSpagetka
force-pushed
the
rosa-initial-integration-test
branch
from
29c0505 to
b926856
Compare
k8s-ci-robot
removed
the
lgtm
|
New changes are detected. LGTM label has been removed. |
|
/retest |
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: