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

feat(k8s): add Helm Chart #500

Merged
merged 14 commits into from
Sep 28, 2023
Merged

feat(k8s): add Helm Chart #500

merged 14 commits into from
Sep 28, 2023

Conversation

apricote
Copy link
Member

What this PR does

Reviewer Notes

This replaces #398 as I did not have push permissions for the branch.

I recommend reviewing by commit, to reduce the noise. This is still a huge PR and if requested I can split it up into multiple.

A lot of the scripts and goreleaser config was shamelessly stolen from hcloud-cloud-controller-manager.

@apricote apricote added the enhancement New feature or request label Sep 14, 2023
@apricote apricote self-assigned this Sep 14, 2023
@apricote apricote requested a review from a team as a code owner September 14, 2023 14:44
Copy link
Member

@jooola jooola left a comment

Choose a reason for hiding this comment

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

I did not finish my review, but here is already a few points:

  • Could we add tests for this ? The templating mixed with yaml feels a bit messy and I fear that it might hide a bunch of bugs. What about generating a set of charts with different .Values and compare it to some snapshots we keep in the repo?
  • Could we reduce the complexity by writing good documentation instead of making the chart overly complex and handling every last field (e.g. not build the image field) ?
  • Some files have an inconsistent naming, for example pbd.yaml or extra-list.yaml. I would also be in favor of using kebab case filenames.
  • Should we introduce a values.schema.json file to validate the values files ?

chart/Chart.yaml Outdated Show resolved Hide resolved
Comment on lines +5 to +6
> [!WARNING]
> The Helm Chart is not yet published and the instructions below will not work until the next release (v2.5.0).
Copy link
Member

Choose a reason for hiding this comment

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

Why not release 2.5.0 right after this is merged ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not want to force our hands one this. I was planning on deploying the Chart to my personal cluster before creating a release, but I can also do that from this PR.

chart/templates/_common_images.tpl Outdated Show resolved Hide resolved
chart/templates/_common_images.tpl Outdated Show resolved Hide resolved
@apricote
Copy link
Member Author

Could we add tests for this ? The templating mixed with yaml feels a bit messy and I fear that it might hide a bunch of bugs. What about generating a set of charts with different .Values and compare it to some snapshots we keep in the repo?

For sure, we already do that with the existing deployment manifest. I added two additional snapshots: default values & production example.

Some files have an inconsistent naming, for example pbd.yaml or extra-list.yaml. I would also be in favor of using kebab case filenames.

Removed extra-list.yaml and the associated functionality (deploying random resources?). Renamed pdb.yaml. Not so sure about the kebap-case, never seen that in a helm chart :D

Should we introduce a values.schema.json file to validate the values files ?

Sounds good :) In the latest push.

chart/.snapshots/example-prod.yaml Outdated Show resolved Hide resolved
chart/templates/_common_images.tpl Outdated Show resolved Hide resolved
chart/values.yaml Outdated Show resolved Hide resolved
@jooola
Copy link
Member

jooola commented Sep 19, 2023

Nice!

@apricote
Copy link
Member Author

apricote commented Sep 27, 2023

I added an additional snapshot test that defines all optional values to make sure everything works. While doing that I found a bunch of features that did not work properly and fixed those.

I still want to do some manual testing before merging.

@apricote
Copy link
Member Author

Manual testing went well. I also updated my personal cluster to use the chart from this branch and everything worked as expected :)

Copy link
Member

@jooola jooola left a comment

Choose a reason for hiding this comment

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

Great effort to build the snapshots tests! 💯

@apricote apricote merged commit c39b138 into main Sep 28, 2023
7 of 8 checks passed
@apricote apricote deleted the helm-chart branch September 28, 2023 13:04
@apricote apricote mentioned this pull request Sep 28, 2023
4 tasks
This was referenced Sep 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Helm chart
3 participants