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 helm unittesting scaffolds #599

Merged
merged 16 commits into from
Oct 31, 2024
Merged

Add helm unittesting scaffolds #599

merged 16 commits into from
Oct 31, 2024

Conversation

dbkegley
Copy link
Contributor

related to #593

Our helm charts are missing tests that can assert that the rendered helm chart templates produce the correct output based on the inputs. This PR sets up unittest scaffolds for all product chart owners to start adding these types of tests

We use the helm unittest plugin for writing tests. Tests live in the charts/*/test directory for each helm chart.

@dbkegley dbkegley requested review from jforest, plascaray and a team October 30, 2024 17:01
@dbkegley dbkegley requested review from a team as code owners October 30, 2024 17:01
Copy link
Contributor

@jforest jforest left a comment

Choose a reason for hiding this comment

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

Need to bump the chart version

✖︎ rstudio-connect => (version: "0.7.12", path: "charts/rstudio-connect") > chart version not ok. Needs a version bump! 

.github/workflows/chart-test.yaml Show resolved Hide resolved
@dbkegley dbkegley requested review from nihara-thomas and removed request for a team October 30, 2024 17:55
@dbkegley dbkegley requested a review from jforest October 30, 2024 18:02
@jspiewak
Copy link
Member

Need to bump the chart version

✖︎ rstudio-connect => (version: "0.7.12", path: "charts/rstudio-connect") > chart version not ok. Needs a version bump! 

Perhaps if the tests directory is added to .helmignore per https://github.com/helm-unittest/helm-unittest?tab=readme-ov-file#get-started then we won't have to bump the chart version when adding tests (which feels like a bad thing)?

@dbkegley dbkegley requested a review from a team as a code owner October 30, 2024 19:41
@dbkegley
Copy link
Contributor Author

Perhaps if the tests directory is added to .helmignore

Looks like changing the .helmignore means we have to bump the version initially, which I guess makes sense because it could impact packaging. I'm going to bump Connect's version here and I'll leave it to other chart owners to decide when they want to make this change in their charts to add tests.

@dbkegley dbkegley force-pushed the kegs-helm-unittests branch from 7b5eeaf to a290c20 Compare October 30, 2024 19:50
@jspiewak
Copy link
Member

Perhaps if the tests directory is added to .helmignore

Looks like changing the .helmignore means we have to bump the version initially, which I guess makes sense because it could impact packaging. I'm going to bump Connect's version here and I'll leave it to other chart owners to decide when they want to make this change in their charts to add tests.

Yeah, makes sense that the addition of .helmignore requires a version bump. I wonder if a change to .helmignore would require and additional bump, my guess is yes. But that changes to anything it is ignoring would not.

@jforest
Copy link
Contributor

jforest commented Oct 30, 2024

Looks like changing the .helmignore means we have to bump the version initially, which I guess makes sense because it could impact packaging. I'm going to bump Connect's version here and I'll leave it to other chart owners to decide when they want to make this change in their charts to add tests.

I don't see the downside in adding .helmignore for each chart now, to make it simpler for everyone else to get started in the future.

Yes, it's a version bump right now which is slightly annoying. But it makes everything easier for those coming after you, they won't need to know to put the .helmignore file in place. They can just get busy writing tests!

@dbkegley
Copy link
Contributor Author

Looks like changing the .helmignore means we have to bump the version initially, which I guess makes sense because it could impact packaging. I'm going to bump Connect's version here and I'll leave it to other chart owners to decide when they want to make this change in their charts to add tests.

I don't see the downside in adding .helmignore for each chart now, to make it simpler for everyone else to get started in the future.

Yes, it's a version bump right now which is slightly annoying. But it makes everything easier for those coming after you, they won't need to know to put the .helmignore file in place. They can just get busy writing tests!

Works for me!

CONTRIBUTING.md Outdated Show resolved Hide resolved
Copy link
Contributor

@bschwedler bschwedler left a comment

Choose a reason for hiding this comment

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

Thank you for putting this together!

So much cleaner and targeted than having an ever-growing collection of values files & golden render output for testing.

Copy link
Contributor

@jforest jforest left a comment

Choose a reason for hiding this comment

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

Thank you @dbkegley! This is a fantastic addition that will make it easier for all the teams to start unit testing their charts, I really appreciate it!

@dbkegley dbkegley merged commit da072f6 into main Oct 31, 2024
7 checks passed
@dbkegley dbkegley deleted the kegs-helm-unittests branch October 31, 2024 20:30
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.

5 participants