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

Migrate from kuttl to chainsaw #181

Merged
merged 10 commits into from
Mar 12, 2024
Merged

Migrate from kuttl to chainsaw #181

merged 10 commits into from
Mar 12, 2024

Conversation

eljohnson92
Copy link
Collaborator

What type of PR is this?
/kind feature

What this PR does / why we need it:
Migrate from kuttl to chainsaw
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

TODOs:

  • squashed commits
  • includes documentation
  • adds unit tests
  • adds or updates e2e tests

@eljohnson92
Copy link
Collaborator Author

@eddycharly I've opened a PR that should close out #177 and #119 with my changes as a branch so we can validate that automation is working as part of the PR. I have the last KUTTL test remove staged locally as well, just doing some testing before pushing it up. let me know if the chainsaw-test changes I made make sense or if there are any suggestions you have

@AshleyDumaine AshleyDumaine added testing Pull requests that improve tests github_actions Pull requests that update GitHub Actions code labels Mar 11, 2024
Copy link
Contributor

@komer3 komer3 left a comment

Choose a reason for hiding this comment

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

Suggestion: I think having a Readme for chainsaw e2e tests would be helpful

@eddycharly
Copy link
Contributor

@eljohnson92 i will take a look after dinner, today has been very busy, sorry

@eddycharly
Copy link
Contributor

@eljohnson92 the tests look good, there's a couple of things we could do to improve readability but it's a very good start!

i hope chainsaw is not too hard to work with :)

@@ -1,11 +1,11 @@
apiVersion: infrastructure.cluster.x-k8s.io/v1alpha1
kind: LinodeObjectStorageBucket
metadata:
name: linodeobjectstoragebucket-sample
name: (join('-', [$namespace, 'backups']))
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of computing this multiple times i would create a test level binding with the computed name

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

is $namespace available at the top binding level? I am seeing this error

=== NAME  chainsaw
    | 19:35:38 | chainsaw | @main | INTERNAL  | ERROR |
        === ERROR
        <nil>: Internal error: variable not defined: $namespace

with this binding def

spec:
  template: true
  bindings:
    - name: bucketName
      value: (join('-', [$namespace, 'backups']))

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right, it's a bug. Fixing it, it will be in the next release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Comment on lines +46 to +56
- name: step-04
try:
- delete:
ref:
apiVersion: infrastructure.cluster.x-k8s.io/v1alpha1
kind: LinodeObjectStorageBucket
name: (join('-', [($namespace), 'backups']))
- name: step-05
try:
- error:
file: 05-errors.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder if you want to run this in a finally block

Comment on lines 43 to 74
- name: step-03
try:
- delete:
ref:
apiVersion: infrastructure.cluster.x-k8s.io/v1alpha1
kind: LinodeVPC
name: ($namespace)
- error:
file: 03-error.yaml
- name: step-04
try:
- script:
env:
- name: TARGET_API
value: api.linode.com
- name: TARGET_API_VERSION
value: v4beta
- name: URI
value: nodebalancers
- name: FILTER
value: '{"label":"($namespace)"}'
content: |
set -e
curl -s \
-H "Authorization: Bearer $LINODE_TOKEN" \
-H "X-Filter: $FILTER" \
-H "Content-Type: application/json" \
"https://$TARGET_API/$TARGET_API_VERSION/$URI"
check:
($error): ~
(json_parse($stdout)):
results: 0
Copy link
Contributor

Choose a reason for hiding this comment

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

same question about the finally block (to make sure it is executed even when the test fails)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if a single step fails does that cause other steps to not run? I see documentation about a finally block within the scope of a single step but not the scope of a test.

Copy link
Contributor

Choose a reason for hiding this comment

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

you're right, if a step fails following steps are not run and finally blocks are per step.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are there any current features that would allow us to always run a step to clean up resources even if previous steps fail?

Copy link
Contributor

Choose a reason for hiding this comment

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

nope, chainsaw will automatically delete all resources it created though, you shouldn't need it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

great, that is good to know

@eljohnson92 eljohnson92 merged commit 01a0179 into main Mar 12, 2024
6 checks passed
@eljohnson92 eljohnson92 deleted the chainsaw-4 branch March 12, 2024 15:38
@eddycharly
Copy link
Contributor

Folks, i see this concludes the kuttl -> chainsaw migration 🎉

Thanks for your support, really appreciated 🙏
I hope you will take advantage of the chainsaw features!

I have one more request for you, we try to maintain a list of adopters, it helps us dedicate some time to the project.

Would you mind opening a PR to add yourself in the list ?

No obligation of course but would be greatly appreciated 🤞

cc @eljohnson92 @bcm820 @AshleyDumaine

@mhmxs
Copy link
Contributor

mhmxs commented Mar 21, 2024

@eddycharly Thank you for your change, i really appreciate it 🙏

@eddycharly
Copy link
Contributor

Hey @mhmxs this is a small world 😂

BTW, i take this opportunity to renew my request #181 (comment)
It would awesome to have you in the adopters list ❤️

amold1 pushed a commit that referenced this pull request May 17, 2024
* chore: migrate from kuttl to chainsaw

Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>

* set bucket name based on namespace


* migrate vpc test from kuttl to chainsaw and remove all kuttl related tooling


---------

Signed-off-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Co-authored-by: Charles-Edouard Brétéché <charles.edouard@nirmata.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update GitHub Actions code testing Pull requests that improve tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants