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: package helm chart on push #74

Merged
merged 13 commits into from
Jan 18, 2024
Merged

Conversation

niklastreml
Copy link
Contributor

@niklastreml niklastreml commented Jan 17, 2024

Motivation

Since we build and publish a docker image for every pr and main push, we should do the same thing for the helm chart.

❯ helm show chart  oci://ghcr.io/caas-team/charts/sparrow --version 0.2.2-commit-3158f2d
Pulled: ghcr.io/caas-team/charts/sparrow:0.2.2-commit-3158f2d
Digest: sha256:4711bb41a28e0f17ab63faa2890a6c5f4f63822b50413ecaa9be94f23cb86f37
apiVersion: v2
appVersion: commit-3158f2d
description: A Helm chart to install Sparrow
icon: 'https://github.com/caas-team/sparrow/blob/main/docs/img/sparrow.png'
keywords:
  - monitoring
maintainers:
  - email: [email protected]
    name: eumel8
    url: 'https://www.telekom.com'
  - email: [email protected]
    name: y-eight
    url: 'https://www.telekom.com'
name: sparrow
sources:
  - 'https://github.com/caas-team/sparrow'
type: application
version: 0.2.2-commit-3158f2d

Changes

Packages the helm chart in the ci workflow on every pr and main commit, and releases it with its version and appVersion set to commit-<GIT_SHORT_SHA>. Ensures that, whenever we build a new docker image, we also have a helm chart available.

For additional information look at the commits.

Tests done

  • ran pipeline
  • helm upgrade -n sparrow sparrow oci://ghcr.io/caas-team/charts/sparrow --version 0.2.2-commit-3158f2d resulted in image ghcr.io/caas-team/sparrow:commit-3158f2d being used, as expected

TODO

  • I've assigned this PR to myself
  • I've labeled this PR correctly

@niklastreml niklastreml self-assigned this Jan 17, 2024
@niklastreml niklastreml added feature Introduces a new feature area/helm Issues/PRs related to the helm chart labels Jan 17, 2024
@niklastreml niklastreml added this to the 0.3.0 milestone Jan 17, 2024
@niklastreml niklastreml marked this pull request as draft January 17, 2024 14:46
@niklastreml niklastreml marked this pull request as ready for review January 17, 2024 15:01
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Show resolved Hide resolved
@y-eight
Copy link
Contributor

y-eight commented Jan 17, 2024

we should not create a helm release for every commit since the artifacts in the oci registry will explode.

i would suggest to include a filter to just build a pre-release/commit helm bundle if at least one file has been changed in chart dir.

@niklastreml
Copy link
Contributor Author

@y-eight I don't think it's an issue tbh. The helm chart is currently 5.2 KiB small. We could push like a thousand charts for every docker image that we build. Don't think its worth the effort

@puffitos
Copy link
Member

TLDR; we should only create charts for new, working and tagged releases of sparrow. When we start doing proper E2E testing, we should re-evaluate how we handle charts and container images for usage with E2E testing. We don't really need a new container on each push and we most definetely don't need a new chart.

My two cents:
Even though size isn't an issue now, but management of the github packages leaves much to be desired. I haven't found a quick alternative to being able to clean up the package repository or create automatic cleanup rules. Yeah scripts do exists to do this and Github does have an API, must it's not something we should spend our time doing right now.

I'm already questioning if it's meaningful to create a container image and pushing it to github for each commit (we only have 500Mi of space for packages and each container is about 12Mi big. We don't even need to push the image to the registry to do the e2e tests (the local image can be pushed into the kubernetes cluster directly).

Do we need this to manually test the helm chart? Since we don't have any test automation, I doubt we're gaining something from having a OCI chart lying around, that we can deploy. I can just build my image locally (it's much quicker) and deploy the current helm chart locally (also quicker).

@niklastreml
Copy link
Contributor Author

Building the helm chart for every image makes testing internally easier, since we can just pull in the right version from our registry without having to fork around it with submodules or something like that in fleet. I'd like to easily test prerelease versions of the image and chart internally without having to build a bunch of workarounds

Copy link
Member

@puffitos puffitos left a comment

Choose a reason for hiding this comment

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

Since we're in the early testing stages of the product, I think it's ok to release a new chart each time. Since we're automatically deploying to our staging environment, as the new charts come in, then it's the lesser evil here to choose the creation of a chart for each commit.

Down the line we should really invest time into a solid E2E suite, which would minimize the need to do deployments to a staging/test cluster first.

@niklastreml
Copy link
Contributor Author

merging this, but definitely agree with you @puffitos

@niklastreml niklastreml merged commit 9dc3eed into main Jan 18, 2024
11 checks passed
@lvlcn-t lvlcn-t deleted the feat/publish-helm-chart-on-master branch January 18, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Issues/PRs related to the helm chart feature Introduces a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants