search

rstudio / helm

Helm Resources for RStudio Products
MIT License
36 stars 28 forks source link

Add helm unittesting scaffolds #599

Closed dbkegley closed 3 weeks ago

dbkegley commented 3 weeks ago

related to https://github.com/rstudio/helm/pull/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.

jspiewak commented 3 weeks ago

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 commented 3 weeks ago

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.

jspiewak commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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!