search

honeycombio / helm-charts

Helm repository and charts for Honeycomb
Apache License 2.0
30 stars 39 forks source link

[chore] add chart testing #193

Closed TylerHelmuth closed 2 years ago

TylerHelmuth commented 2 years ago

Which problem is this PR solving?

Currently all tests of the helm charts have to be done manually. This can be time consuming and it can be difficult to cover all useful test scenarios

Short description of the changes

Adds a new circleci workflow to use chart-testing to test the helm charts. This allows automatic detection of changed charts and then installs all the values.yamls in the ci folder, one at a time, to ensure the chart is installable.

How to verify that this has the expected result

copious amounts of trial and error (118 to be exact) lol

masonjlegere commented 2 years ago

Nice! Probably unrelated as this seems pretty e2e but I've used this plugin for setting up fast unit tests without having to setup minikube/k3d etc

edit: Ah nevermind - saw this issue and figured it's related https://github.com/honeycombio/helm-charts/pull/189 :)

TylerHelmuth commented 2 years ago

@masonjlegere thanks for the suggestion, I've not heard of that one before. I've always used chart-testing to test install helm charts. Annoyingly it does require minikube and kubectl and helm, but I like that it actually does an install of all the scenarios in the ci folder.

This PR would be useful for the PR you linked, but it'll be useful for all PRs going forward as well, especially once we flush out the test scenarios.

TylerHelmuth commented 2 years ago

@MikeGoldsmith it will be good to run the tests one more time before releasing, but we'll need to think about how to do this. I don't think adding the test job I wrote will work in this instance since it is taking advantage of chart-testing's ability to detect changes between the PR's chart and the main branch's chart. For the release workflow, it won't find any changes and won't run any tests.

I think we should still move forward with this PR to get automated tests on our PRs while we continue to iterate on how to best test before the release is pushed.

robbkidd commented 2 years ago

chart-testing […] detect[s] changes between the PR's chart and the main branch's chart

Would "test-modified-charts" be a better name for this job? That could manage the reader's expectations and be less surprising that the job doesn't test anything if there are no changes to a chart in a PR.

The "test" job in the majority of our other projects involves testing everything, modified and unmodified.

MikeGoldsmith commented 2 years ago

@MikeGoldsmith it will be good to run the tests one more time before releasing, but we'll need to think about how to do this. I don't think adding the test job I wrote will work in this instance since it is taking advantage of chart-testing's ability to detect changes between the PR's chart and the main branch's chart. For the release workflow, it won't find any changes and won't run any tests.

I think we should still move forward with this PR to get automated tests on our PRs while we continue to iterate on how to best test before the release is pushed.

Thanks for the clarification on how the tests are expected to run and I agree it doesn't makes sense to make the test step a prerequisite for releasing here 👍🏻