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

chore: migrate from kuttl to chainsaw #2630

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

eddycharly
Copy link
Contributor

This PR migrates e2e tests from kuttl to chainsaw.

Chainsaw was created because kuttl stopped to be actively maintained (it's actually a single maintainer now).

We are also solving most of the kuttl limitations that exist today and wrote a command to automatically migrate from kuttl to chainsaw. It supercharges e2e tests with better logs, config maps assertions, assertions trees and many more things 💪

This is a relatively young project but it has already been adopted by a couple of e2e tests heavy projects (kyverno, keptn, fairwinds rbac-manager, grafana-operator, redis-operator and others).

To give more context, you can see similar PRs at:

Sadly, kuttl haven't done a release since jan 2023.

Here you can find a good video showcasing chainsaw: https://www.youtube.com/watch?v=Ejof-wtAdQM&ab_channel=CamilaMacedo

To make things more concrete, there are a couple of issues in this repo, a few files are not valid and kuttl doesn't complain because unmarshalling is not strict so the non-valid fields are simply ignored. This is a problem because the behaviour of the tests is not the expected one, for example:

If you need more infos 👇

@eddycharly eddycharly requested a review from a team February 15, 2024 20:05
@eddycharly
Copy link
Contributor Author

Replaces #2607 🙈

@eddycharly eddycharly force-pushed the chainsaw-2 branch 2 times, most recently from 3760863 to 3a40714 Compare February 15, 2024 20:28
@eddycharly
Copy link
Contributor Author

For reference, we should really avoid this kind of thing

apiVersion: kuttl.dev/v1beta1
kind: TestStep
commands:
- command: kubectl -n $NAMESPACE create clusterrolebinding default-view-$NAMESPACE --clusterrole=promreceiver-allocatorconfig --serviceaccount=$NAMESPACE:ta

Chainsaw doesn't know anything about resources created with kubectl and therefore won't clean them up.

Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
@jaronoff97 jaronoff97 added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Feb 15, 2024
@swiatekm
Copy link
Contributor

For reference, we should really avoid this kind of thing

apiVersion: kuttl.dev/v1beta1
kind: TestStep
commands:
- command: kubectl -n $NAMESPACE create clusterrolebinding default-view-$NAMESPACE --clusterrole=promreceiver-allocatorconfig --serviceaccount=$NAMESPACE:ta

Chainsaw doesn't know anything about resources created with kubectl and therefore won't clean them up.

We can use templating for it, right? That can happen as a follow-up PR though.

@eddycharly
Copy link
Contributor Author

eddycharly commented Feb 16, 2024

@swiatekm-sumo we can use templating indeed, will do in a follow-up.

image

This one is already big enough 😂

Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

LGTM overall, left some minor comments and questions about the Makefile and potential improvements to the tests using chainsaw's features.

@@ -118,11 +118,11 @@ KUBEBUILDER_ASSETS=$(./bin/setup-envtest use -p path 1.23) go test ./pkg...

### End to end tests

To run the end-to-end tests, you'll need [`kind`](https://kind.sigs.k8s.io) and [`kuttl`](https://kuttl.dev). Refer to their documentation for installation instructions.
To run the end-to-end tests, you'll need [`kind`](https://kind.sigs.k8s.io) and [`chainsaw`](https://kyverno.github.io/chainsaw). Refer to their documentation for installation instructions.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can just delete this line and adjust the next one. All of these tools are now installed automatically by the make targets.

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 will do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow up PR if it's ok.

Comment on lines +410 to +413
@{ \
set -e ;\
go install github.com/kyverno/chainsaw@v0.1.4 ;\
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We have a small utility to make this nicer.

Suggested change
@{ \
set -e ;\
go install github.com/kyverno/chainsaw@v0.1.4 ;\
}
$(call go-get-tool,$(CHAINSAW), github.com/kyverno/chainsaw,v0.1.4)

Can we also move the version to its own variable, like the other tools?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow up PR if it's ok.

Makefile Show resolved Hide resolved
Copy link
Contributor

@swiatekm swiatekm left a comment

Choose a reason for hiding this comment

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

LGTM, we can take care of minor fixes in a follow up. Thanks a lot for your work on this contribution @eddycharly !

@eddycharly
Copy link
Contributor Author

Ok, i'll address those comments in follow ups once this one merges :)

@jaronoff97 jaronoff97 merged commit ab2c3c0 into open-telemetry:main Feb 16, 2024
29 of 30 checks passed
@jaronoff97
Copy link
Contributor

Incredible work here @eddycharly !!! Thank you very much for your contributions. I'm looking forward to those follows to clean this up further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Skip Changelog PRs that do not require a CHANGELOG.md entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants