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

Add tool for e2e test cluster setup and cleanup #313

Merged
merged 8 commits into from
Aug 23, 2023

Conversation

pablochacin
Copy link
Collaborator

Description

Fixes #311

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make test) and all tests pass.
  • I have run relevant e2e test locally (make e2e-xxx for agent, disruptors, kubernetes or cluster related changes)
  • Any dependent changes have been merged and published in downstream modules

Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
@pablochacin pablochacin force-pushed the e2e-test-setup-cleanup branch 3 times, most recently from f652a1f to 59edf00 Compare August 23, 2023 08:37
@pablochacin pablochacin marked this pull request as ready for review August 23, 2023 11:51
@pablochacin pablochacin requested a review from nadiamoe August 23, 2023 11:51
Copy link
Member

@nadiamoe nadiamoe left a comment

Choose a reason for hiding this comment

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

LGTM, left just a few cosmetic comments

Comment on lines 40 to 41
cmd.Flags().StringArrayVarP(&images, "image", "i", cluster.DefaultE2eClusterConfig().Images,
"additional image to pre-load in the cluster")
Copy link
Member

Choose a reason for hiding this comment

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

Does cobra handle array args as comma-separated list, or as specify the flag multiple times?
I never remember which is which, so I would suggest to specify this in the help message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. It supports both. I will add this to the comment.


> Note: if you are testing changes in the agent, be sure you generate the image and [upload it to the cluster](#building-the-xk6-disruptor-agent-image) using kind

When you don't longer needs the cluster, you can delete it using kind:
When you don't longer needs the cluster, you can delete it using the [e2e-cluster tool](#e2e-cluster-tool) or `kind`:
Copy link
Member

Choose a reason for hiding this comment

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

Double space

Suggested change
When you don't longer needs the cluster, you can delete it using the [e2e-cluster tool](#e2e-cluster-tool) or `kind`:
When you don't longer needs the cluster, you can delete it using the [e2e-cluster tool](#e2e-cluster-tool) or `kind`:

This behavior is controlled by passing the `WithKeepOnFail` option when creating the namespace or by setting `E2E_KEEPONFAIL=true` in the environment when running an e2e test.
Copy link
Member

Choose a reason for hiding this comment

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

Other env vars are set to 0 or 1, but this one we set it to true. IIRC both are okay as we use strconv.ParseBool which accepts both, but I think it would be good to stick to one or the other in the examples.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. It should also be 1.

@pablochacin pablochacin force-pushed the e2e-test-setup-cleanup branch from 97f4e36 to e32ca86 Compare August 23, 2023 14:29
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
Signed-off-by: Pablo Chacin <pablochacin@gmail.com>
@pablochacin pablochacin force-pushed the e2e-test-setup-cleanup branch from e32ca86 to 1af2329 Compare August 23, 2023 14:37
@pablochacin pablochacin merged commit b600d2b into main Aug 23, 2023
@pablochacin pablochacin deleted the e2e-test-setup-cleanup branch August 23, 2023 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement e2e setup/cleanup command
2 participants