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: matrix testing pre-existing k3d clusters #944

Merged
merged 13 commits into from
Sep 27, 2024
Merged

Conversation

catsby
Copy link
Collaborator

@catsby catsby commented Sep 26, 2024

Description

Cleanup some of our GitHub Actions, and run (some of?) our e2e tests in a matrix where one scenario we have k3d installed and a cluster created, and another with k3d installed and no cluster created.

Note about our setup-golang action: I noticed that we were using setup-go to install go 1.21.x, however the setup-go action does not list the .x syntax as supported. Furthermore our go.mod file says go 1.22.4, so the setup go action was setting 1.21.x, and then immediately switching to go 1.22.4 because that's what our go.mod says. The result was a long list of errors trying to use 1.22 because it was being pulled from cache but conflicted with the 1.21 we just setup.

See here for example: https://github.com/defenseunicorns/uds-cli/actions/runs/11037302233/job/30657907519#step:3:102

Note about our setup-from-previous action: I noticed that every invocation was preceded by a checkout action, and the first step of setup-from-previous was also a checkout.

See here and here for example of trying to checkout the repo 2x in the same job

Related Issue

Fixes #940

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

@catsby catsby marked this pull request as ready for review September 26, 2024 16:23
- existing-cluster: without-cluster
create-cluster: 'false'
- existing-cluster: with-cluster
create-cluster: 'true'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Open to suggestions on naming here, but basically wanted to convey that one matrix test operates by installing k3d and creating a cluster, while the other specifically does not create the cluster.

Copy link
Collaborator

@mjnagel mjnagel Sep 27, 2024

Choose a reason for hiding this comment

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

Would it be clearer if this were just existing-cluster as a true/false and then pass the inverse to the action below? I'm not a big fan of the second matrix parameter here when it's really the same as the first just different name and truthy vs descriptive string.

Edit: forgot that the matrix value gets used in the job name when viewing it, so true/false would be a bit weird and unhelpful there. But maybe could still delete the second matrix param and use a string equality check below to pass true/false into the action?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

forgot that the matrix value gets used in the job name when viewing it, so true/false would be a bit weird and unhelpful there.

Maybe this depends on how it's named? In this case, the label is being used, not the value:

with-cluster

But maybe I'm misunderstanding what you're saying here, I'm not fully versed in GitHub Actions these days

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah ok re-reading this I believe you're referring to existing-cluster: [with-cluster, without-cluster], I thought you were referring to the include: part.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Added suggestions that I think would work? Would also need to do the same on the release test of course, but wanted to add for your thoughts.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@mjnagel I applied your suggestions, thanks! At first run I found the logic was backwards (with- did not create a cluster, but without- did) so I swapped it in e1c460c which I believe is correct now. Watching CI run..

@catsby catsby changed the title chore: attempt at matrix testing pre-existing k3d clusters chore: matrix testing pre-existing k3d clusters Sep 26, 2024
.github/workflows/nightly-uds-core.yaml Outdated Show resolved Hide resolved
.github/workflows/nightly-uds-core.yaml Outdated Show resolved Hide resolved
Copy link
Collaborator

@UncleGedd UncleGedd left a comment

Choose a reason for hiding this comment

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

love the matrix strategy 🚀

@catsby catsby merged commit b6422ab into main Sep 27, 2024
16 checks passed
@catsby catsby deleted the matrix-k3d-cluster-create branch September 27, 2024 22:02
catsby added a commit that referenced this pull request Oct 1, 2024
* main:
  chore(deps): update actions/checkout action to v4.2.0 (#943)
  fix: add suffix to smoke test logs (#947)
  chore: matrix testing pre-existing k3d clusters (#944)
  chore: remove nascent state tracking (#942)
  fix: ensure runtime bins are included in releases (#939)
  chore: update uds ui docs (#937)
  fix: update maru-runner to silence info log (#925)
  chore: manually bump uds-runtime to v0.4.0 (#938)
  chore(deps): update module github.com/prometheus/common to v0.59.1 (#877)
  fix(deps): update kubernetes packages to v0.31.1 (#932)
  fix(deps): update module github.com/defenseunicorns/maru-runner to v0.2.3 (#933)
  fix(deps): update module github.com/defenseunicorns/pkg/oci to v1.0.2 (#934)
  chore(deps): update defenseunicorns/uds-common action to v0.13.0 (#935)
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.

Ensure smoke tests cover both pre-existing cluster and no pre-existing cluster test cases
3 participants