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

Simplify cluster instantiation in tests #407

Merged
merged 14 commits into from
Mar 21, 2022

Conversation

RothAndrew
Copy link
Contributor

@RothAndrew RothAndrew commented Mar 16, 2022

Description

Simplifies the handling of kubernetes clusters from this logic:

  1. Determine if a cluster is already running, and if it is, use it instead of creating one
  2. If a cluster isn't running, create one using the Golang libraries for Kind and/or K3d, using a hard-coded KUBECONFIG file (~/.kube/config)
    • If TESTDISTRO is not set, run the tests twice, once on KinD, and again on K3d
    • If TESTDISTRO is set to either kind or k3d only do that one
  3. Run the tests
  4. Destroy the cluster if it was created and delete the ~/.kube/config file

To this:

  1. Expect TESTDISTRO env var to be set, with value one of provided, kind, k3d, or k3s and error out if it isn't
  2. If TESTDISTRO is
    • kind or k3d -- create that cluster using the associated golang library, using a temporary file for the KUBECONFIG.
    • provided -- Use the provided cluster, using the existing KUBECONFIG
    • k3s -- Use zarf init --components k3s to create the cluster and zarf destroy to delete it (done as part of each test so that each test gets a fresh cluster)
  3. Run the tests
  4. If not provided or k3s, destroy the cluster and temporary KUBECONFIG file

The pipeline will provide the KinD cluster, and will expect the tests to create the K3d and K3s clusters

Related Issue

Fixes #406
Related to #373 (except for needing a new e2e test)

Type of change

  • Refactor (non-breaking change which preserves functionality and increases maintainability)

Checklist before merging

  • Tests have been added/updated as necessary (add the needs-tests label)
  • Documentation has been updated as necessary (add the needs-docs label)
  • An ADR has been written as necessary (add the needs-adr label) [ 1 2 3 ]
  • (Optional) Changes have been linted locally with golangci-lint. (NOTE: We haven't turned on lint checks in the pipeline yet so linting may be hard if it shows a lot of lint errors in places that weren't touched by changes. Thus, linting is optional right now.)

@RothAndrew RothAndrew added needs-adr needs-docs PR Label - Docs required to merge needs-tests PR Label - Tests required to merge labels Mar 17, 2022
@RothAndrew
Copy link
Contributor Author

I don't think an ADR is needed here, but happy to write one if the team feels it is needed.

Still need to update docs, will do that next after I ensure this is working how I expect it to.

Since the whole thing is about tests, I'll go ahead and pull that label

@RothAndrew RothAndrew removed needs-adr needs-tests PR Label - Tests required to merge labels Mar 17, 2022
@RothAndrew
Copy link
Contributor Author

Updated description based on offline conversation with @YrrepNoj

@RothAndrew
Copy link
Contributor Author

@YrrepNoj ready for you to look at this based on what we talked about. Still need to update docs but I think it's mostly there.

@RothAndrew RothAndrew requested a review from YrrepNoj March 19, 2022 19:59
@RothAndrew RothAndrew self-assigned this Mar 19, 2022
@RothAndrew RothAndrew mentioned this pull request Mar 19, 2022
4 tasks
@RothAndrew RothAndrew removed the needs-docs PR Label - Docs required to merge label Mar 20, 2022
@RothAndrew RothAndrew changed the title WIP: Simplify cluster instantiation in tests Simplify cluster instantiation in tests Mar 20, 2022
@RothAndrew
Copy link
Contributor Author

Ready for review and merge

YrrepNoj
YrrepNoj previously approved these changes Mar 21, 2022
Copy link
Contributor

@YrrepNoj YrrepNoj left a comment

Choose a reason for hiding this comment

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

Worked together in person to address comments.

@RothAndrew RothAndrew enabled auto-merge (squash) March 21, 2022 17:50
@RothAndrew RothAndrew disabled auto-merge March 21, 2022 18:09
@jeff-mccoy
Copy link
Contributor

Reading back through all this rn after getting back from spring break, give me a few to verify too before merge please. Also note @RothAndrew you'll probably want to squash your commits as you have an unsigned one blocking merge.

Makefile Show resolved Hide resolved
@RothAndrew
Copy link
Contributor Author

Also note @RothAndrew you'll probably want to squash your commits as you have an unsigned one blocking merge.

The commit is signed, but for some reason GitHub is not seeing it as verified, even though the same GPG key was used for it as all the other commits. I'm having trouble figuring out how to squash/force-push to fix since there's a Merge Commit in the middle.

Copy link
Contributor

@jeff-mccoy jeff-mccoy left a comment

Choose a reason for hiding this comment

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

Overall I think this will be an improvement to the testing flow, but some custom logic may need to use the main codebase instead and we should be careful to make sure we're not doing something different than how a normal user would use Zarf as that's the point of E2E is to be as real as possible. Thanks for working this.

@RothAndrew RothAndrew enabled auto-merge (squash) March 21, 2022 19:35
@RothAndrew RothAndrew requested a review from jeff-mccoy March 21, 2022 19:43
@RothAndrew RothAndrew merged commit 0306af7 into master Mar 21, 2022
@RothAndrew RothAndrew deleted the feature/simplify-cluster-instantiation-in-tests branch March 21, 2022 22:15
Noxsios pushed a commit that referenced this pull request Mar 8, 2023
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.

Simplify Kubernetes cluster instantiation in tests
3 participants