Skip to content
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

Drop legacy catalog for tests #291

Merged
merged 3 commits into from Dec 14, 2022
Merged

Drop legacy catalog for tests #291

merged 3 commits into from Dec 14, 2022

Conversation

davidcassany
Copy link
Contributor

Signed-off-by: David Cassany dcassany@suse.com

@davidcassany davidcassany requested a review from a team December 13, 2022 08:31
@davidcassany davidcassany marked this pull request as draft December 13, 2022 08:31
@github-actions github-actions bot added the area/tests test related changes label Dec 13, 2022
@davidcassany
Copy link
Contributor Author

Requires rancher-sandbox/ele-testhelpers#25

@github-actions github-actions bot added the area/build build related changes label Dec 13, 2022
@davidcassany davidcassany marked this pull request as ready for review December 13, 2022 10:41
@davidcassany davidcassany enabled auto-merge (squash) December 13, 2022 10:46
EventuallyWithOffset(1, func() error {
return k.ApplyYAML(fleetNamespace, name, mr)
return k.ApplyJSON("", name, mr)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also use controller runtime client to create/update/delete object as we did here https://github.com/rancher/elemental-operator/blob/main/tests/e2e/machineregistration_test.go#L95

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, let me give it a try. I guess not all kubectl calls of this test can be replaced though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexander-demicev see my last commit, I used the controller runtime to create the CRDs instead of the kubectl wrapper with JSON inputs, however I could not manage to use the controller runtime to fetch CRDs that are not defined by this controller API, which is problematic for this tests as it operates with CRDS from github.com/rancher/system-upgrade-controller/pkg/apis/upgrade.cattle.io/v1. For instance if trying to adapt

func getPlan(s string) (up *upgradev1.Plan, err error) {
up = &upgradev1.Plan{}
err = kubectl.GetObject(s, cattleSystemNamespace, "plan", up)
return
}

I get a no kind is registered for the type v1.Plan in scheme error, which I assume is caused because system-upgrade-controller is not based on controller runtime. Do you know if this is something we can easily circumvent? Otherwise I am more leaning towards dropping my latest commit and do not use the controller runtime client for this test. I dislike using two different k8s clients on the same piece of code, specially for tests it turns to be confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I finally dropped the last commit and for now I did not use the client from controller runtime. As I could not figure out a proper way to use it for all cases required here in this test.

Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
@davidcassany davidcassany marked this pull request as ready for review December 14, 2022 16:20
@davidcassany davidcassany enabled auto-merge (squash) December 14, 2022 16:21
@davidcassany davidcassany merged commit af83eea into main Dec 14, 2022
@davidcassany davidcassany deleted the drop_legacy_test_catalog branch December 14, 2022 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build build related changes area/tests test related changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants