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 #786

Merged
merged 8 commits into from
Feb 26, 2024
Merged

Conversation

eddycharly
Copy link
Contributor

Migrate from kuttl to chainsaw.

Chainsaw is similar to kuttl but a lot more flexible and actively maintained.

If you need more infos 👇

Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
@eddycharly
Copy link
Contributor Author

BTW this file is not valid

name: tempo-redmetrics
apiVersion: kuttl.dev/v1beta1
kind: TestAssert
commands:
- command: /bin/sh -c "kubectl get --namespace $NAMESPACE tempo redmetrics -o jsonpath='{.status.conditions[?(@.type==\"Ready\")].status}' | grep True"

@pavolloffay
Copy link
Collaborator

hi @eddycharly thanks for the PR. Is there any statement in kuttl project about not being activelly maintained?

chainsaw seems to be only 3-4 old project (looking at releases) which concerns me. Is there any evidence of adoption by operaotr-sdk/kubebuilder?

@eddycharly
Copy link
Contributor Author

eddycharly commented Feb 8, 2024

Hi @pavolloffay

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

We are also solving most of the kuttl limitations that exist today and wrote a command to automatically migrate from kuttl to chainsaw.

We presented chainsaw to kubebuilder/operator-sdk folks last week and are discussing how to collaborate:

I agree this is a relatively young project but it has already been adopted by a couple of e2e tests heavy projects (kyverno, keptn, fairwinds, opentelementry-operator is the next one, and there is a PR for grafana-operator grafana/grafana-operator#1406).

This is a solid kuttl replacement, i can change the PR to run both tools side by side if you prefer but maintaining two identical test suites is a pain.

@andreasgerstmayr
Copy link
Collaborator

andreasgerstmayr commented Feb 8, 2024

This is a solid kuttl replacement, i can change the PR to run both tools side by side if you prefer but maintaining two identical test suites is a pain.

Could this be a drop-in replacement? I.e. we can just replace the kuttl binary with chainsaw, and it works as-is?
Once we add chainsaw-only features, we cannot migrate back anymore of course. But it'd make adoption much easier. For example podman did this, you can do alias docker=podman and it just works for most users.

@eddycharly
Copy link
Contributor Author

Could this be a drop-in replacement?

Unfortunately no, cli args are not exactly the same and chainsaw doesn't support TestStep or TestAssert resources.
That's the reason why we created a migration command.

It would be nice to let the workflows run and look at the result, happy to discuss it more if i have a chance to convince you :)

Here is the recording of the last KB meeting where we presented chainsaw if you want to see it in action https://www.youtube.com/watch?v=Ejof-wtAdQM

@eddycharly
Copy link
Contributor Author

eddycharly commented Feb 13, 2024

Grafana-operator adopted chainsaw today grafana/grafana-operator#1406 🎉 ❤️

Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
@eddycharly
Copy link
Contributor Author

@pavolloffay @andreasgerstmayr did you have a chance to look at this ?

@eddycharly
Copy link
Contributor Author

eddycharly commented Feb 15, 2024

@pavolloffay I think we met today during the opentelemetry-operator SIG meeting. I hope you loved chainsaw :)

@pavolloffay
Copy link
Collaborator

LGTM I am fine with merging this. @andreasgerstmayr WDYT?

@codecov-commenter
Copy link

codecov-commenter commented Feb 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 75.31%. Comparing base (10f1c2b) to head (dd7cc63).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #786   +/-   ##
=======================================
  Coverage   75.31%   75.31%           
=======================================
  Files          89       89           
  Lines        6369     6369           
=======================================
  Hits         4797     4797           
  Misses       1342     1342           
  Partials      230      230           
Flag Coverage Δ
unittests 75.31% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

- command:
args:
- -c
- '! kubectl get --namespace $NAMESPACE configmap tempo-simplest -o jsonpath=''{.data.tempo\.yaml}''
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the var $NAMESPACE work here same as KUTTL ? I need to check how the Chainsaw tests work when run from inside a pod as the OpenShift CI uses Prow CI to run the jobs and the tests are run from inside a pod.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes $NAMESPACE works the same as in KUTTL.
Most scripts can be replaced with asserts/apply operations in chainsaw, i will look deeper at your tests once this PR merges to remove scripts when possible.

@eddycharly
Copy link
Contributor Author

I'll fix the failing tests soon, thanks for triggering the workflow.

Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
@eddycharly
Copy link
Contributor Author

@pavolloffay @IshwarKanse i pushed a fix 🤞

@eddycharly
Copy link
Contributor Author

I'm working on a PR to fix the OpenTelemetry Operator tests, also to get them working with OpenShift CI.

@IshwarKanse please ping me on the PR, there's something i don't understand fully here.

@eddycharly
Copy link
Contributor Author

We released v0.1.6 today, fixing an important issue related to concurrency, shall i update this PR ?

@IshwarKanse
Copy link
Contributor

@eddycharly yeah sure. Please update the version. Thanks.

Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
@eddycharly
Copy link
Contributor Author

@IshwarKanse done

@rubenvp8510
Copy link
Collaborator

I'm fine with this change.

@eddycharly
Copy link
Contributor Author

I took the time to remove scripts from upgrade tests, converting

    - script:
        content: kubectl get --namespace $NAMESPACE tempo simplest -o jsonpath='{.status.conditions[?(@.type=="Ready")].status}' | grep True

to an assertion tree equivalent

    - assert:
        resource:
          apiVersion: tempo.grafana.com/v1alpha1
          kind: TempoStack
          metadata:
            name: simplest
          status:
            (length(conditions[?(type == 'Ready' && status == 'True')]) == 0): false

@eddycharly
Copy link
Contributor Author

eddycharly commented Feb 21, 2024

This would work too, maybe better than using length

    - assert:
        resource:
          apiVersion: tempo.grafana.com/v1alpha1
          kind: TempoStack
          metadata:
            name: simplest
          status:
            "(contains(conditions[*].{type: type, status: status}, {type: 'Ready', status: 'True'}))": true

@eddycharly eddycharly force-pushed the chainsaw branch 2 times, most recently from b4c1412 to cf237df Compare February 21, 2024 18:01
@frzifus frzifus enabled auto-merge (squash) February 22, 2024 15:40
@IshwarKanse
Copy link
Contributor

@eddycharly Can you resolve this so that we can merge this PR. Auto-merge is enabled.

- NoCluster false
Loading tests...
Error: failed to parse document (spec.steps[8].try[2].get: Invalid value: value provided for unknown field
spec.steps[9].try[1].get: Invalid value: value provided for unknown field
spec.steps[9].try[2].get: Invalid value: value provided for unknown field)
make: *** [Makefile:372: e2e-upgrade] Error 1
Error: Process completed with exit code 2.

Once merged I'll retest on a OpenShift cluster.

@eddycharly
Copy link
Contributor Author

@IshwarKanse 👀

auto-merge was automatically disabled February 22, 2024 16:29

Head branch was pushed to by a user without write access

@eddycharly
Copy link
Contributor Author

@IshwarKanse can you retry ?

I would be in, but we may want to bring this up on a sync meeting before pressing the button?

I thought it was not going to be merged until a meeting happens 🤷

Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
@eddycharly
Copy link
Contributor Author

eddycharly commented Feb 24, 2024

This would work too, maybe better than using length

    - assert:
        resource:
          apiVersion: tempo.grafana.com/v1alpha1
          kind: TempoStack
          metadata:
            name: simplest
          status:
            "(contains(conditions[*].{type: type, status: status}, {type: 'Ready', status: 'True'}))": true

Hey folks, i didn't like the solution above and needed something similar while working on something else, there's a better solution:

    - assert:
        resource:
          apiVersion: tempo.grafana.com/v1alpha1
          kind: TempoStack
          metadata:
            name: simplest
          status:
            (conditions[?type == 'Ready']):
            - status: 'True'

@eddycharly
Copy link
Contributor Author

If one can trigger the workflow 🙏

What are the next steps ? Any idea when this could potentially land in ? 🤞

@IshwarKanse
Copy link
Contributor

@eddycharly We discussed it internally and decided to merge. cc @frzifus @andreasgerstmayr

@frzifus frzifus merged commit b232fec into grafana:main Feb 26, 2024
11 checks passed
@eddycharly eddycharly deleted the chainsaw branch February 26, 2024 08:18
@eddycharly
Copy link
Contributor Author

Thanks folks!
I'm vacationing this week, i will see how to improve tests and open follow up PRs next week.

@eddycharly
Copy link
Contributor Author

BTW if something breaks or you need help with anything feel free to ping me on slack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants