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

ci: adjust helm chart appVersion before creating release #223

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

lvlcn-t
Copy link
Collaborator

@lvlcn-t lvlcn-t commented Nov 17, 2024

Motivation

To ensure that the Helm chart's appVersion is correctly set before creating a new release. Additionally, the chart maintainers were updated, and the appVersion was bumped to keep the chart metadata current and accurate.

Changes

  • Adjusted the Helm chart appVersion in the CI pipeline to update it before creating a release.
  • Updated the chart maintainers to reflect the current maintainers.
  • Bumped the appVersion to align with the release process.

For additional information look at the commits.

Tests done

  • Unit tests succeeded - not applicable
  • E2E tests succeeded - not applicable

TODO

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

@lvlcn-t lvlcn-t added bug Something isn't working housekeeping area/helm Issues/PRs related to the helm chart area/ci Issues/PRs related to github actions labels Nov 17, 2024
@lvlcn-t lvlcn-t self-assigned this Nov 17, 2024
@puffitos puffitos removed the bug Something isn't working label Nov 18, 2024
@puffitos
Copy link
Collaborator

While I appreciate the effort of automating the release process, this doesn't prevent us from creating faulty tags, as it only sets the tag before creating the release. In case we forget to set the appVersion ourselves to the correct one, we've created a dissonance between the source of truth (our codebase) and the created artifact (the built helm chart pushed to the various registries).

IMO we should select only one way of what the chart looks like:

  • everything is templated and only filled upon release (like cert-manager does)
  • nothing is filled by scripts during the CI of a tagged push. Everything is done via pre-commit; if you're currently on a stable tag, check that the appVersion matches it. If not, block the push.

We should select one and stick with it. Not mix and match just to make sure a chart with an incorrect appVersion is pushed.

@puffitos
Copy link
Collaborator

@lvlcn-t I've removed the bug label, as this isn't a bug. Everything is working normally. It's a nice to have enhancement.

@lvlcn-t
Copy link
Collaborator Author

lvlcn-t commented Nov 19, 2024

@puffitos I understand your concerns about the release process and the potential for creating faulty tags. I agree that we should strive for a clear and reliable process using a single source of truth.

New Behavior

  • Automated Versioning: I've updated the release toolchain so that both the Helm chart's version and appVersion fields are automatically set to match the most current git tag in the correct format for each field. This means that every time we create a release, the chart version will be the same as the application version.

Benefits

  • Reduce Human Error: Manually maintaining the chart version is error-prone and can easily be overlooked, as we've seen with the appVersion field falling behind by several versions. Automating this ensures that our chart's versions are always up-to-date and consistent with the application.

  • Simplify the Release Process: By unifying the versioning, we eliminate the need to manage separate versions for the application and the chart. This reduces complexity and potential confusion.

  • Consistency Across Artifacts: Having the same version for both the application and the chart ensures that users can easily correlate the chart with the application release it deploys.

Addressing Concerns

  • Faulty Tags: Automating the versioning doesn't completely prevent the creation of faulty tags, but it reduces the risk by ensuring that the chart's appVersion and version fields are always aligned with the Git tag at the point of release. If the tag is incorrect, it will be consistently incorrect across all artifacts, making it easier to identify and rectify.

  • Single Source of Truth: By deriving versions directly from Git tags, we establish Git as the single source of truth for versioning. This aligns the codebase, Docker images, and Helm charts under the same versioning scheme.

Why Not Use Pre-Commit Hooks or Manual Updates?

  1. Pre-Commit Hooks Are Optional: Pre-commit hooks are optionally installed by the developer. This can lead to inconsistencies.

  2. Inability to Run on Tag Pushes Only: Pre-commit hooks can't be configured to run exclusively on tag pushes, which is essential for our versioning strategy.

  3. CI-Based Commits Are Problematic: Pushing changes back to the main branch from CI might introduce security risks and complicate the workflow, especially when dealing with generated files like documentation. It'd require deleting a faulty tag, and (force) pushing a new one with new commits to fix the discrepancy between the tag and the chart version.

Alternative Considerations

  • Templating Everything: While templating and filling values upon release (as cert-manager does) is an option, it introduces additional complexity into our build process and requires significant changes to our current setup.

  • Manual Version Management: Relying on developers to manually update versions increases the risk of discrepancies and is prone to being forgotten, as we've experienced.

Conclusion

I acknowledge that setting the chart version to match the application version may not fully comply with Helm's recommendation, which suggests incrementing the chart version whenever the chart itself changes, regardless of the application version. However, given the practical challenges and the goal of maintaining consistency, I believe this approach serves our needs better. Please let me know your thoughts or if you'd like to discuss this further.

@lvlcn-t lvlcn-t force-pushed the fix/chart-appVersion branch from a0d108b to a0a5dc8 Compare November 19, 2024 00:38
@puffitos
Copy link
Collaborator

How is git the source of truth? The changes aren't in git! The chart version and the appVersion are 0.

Now you moved to replacing both versions using the makefile additionally, doubling down on the approach you've decided to take.

Creating a tag and making a release is a simple task, I cannot justify the time and effort put into automating any of this. I'm afraid we are over-engineering the CI pipeline. There are only two variables (the versions) which are important and the tag itself. We don't have to automate everything. Our time should be spent with other tasks IMO.

@lvlcn-t
Copy link
Collaborator Author

lvlcn-t commented Nov 19, 2024

@puffitos Then how would you fix/prevent a new release that hasn't updated the versions in the Chart.yaml with the previously mentioned limitations of pre-commit hooks & CI pushes?
As already mentioned above, we have done some mistakes in the past with the helm chart versioning:

  1. We never bumped the chart version even though we changed the chart. Bumping the appVersion field of the chart is a change to the helm chart and also needs the helm chart version to change, as mentioned by the documentation of a newly generated chart:
# This is the chart version. This version number should be incremented each time you make changes
# to the chart and its templates, including the app version.
# Versions are expected to follow Semantic Versioning (https://semver.org/)
version: 0.1.0
  1. We often forgot to even bump the appVersion field when creating a new release. You can see this by looking at the git diff of the PR. The appVersion field was still set to v0.3.2 thus every installed chart without an override to the image.tag used this old image instead of the latest one.
  2. Coupling the appVersion and version fields will ensure that both are directly derived from the current git tag. I know that this is not ideal because it's against the purpose of the version field but how else do we ensure that the version is bumped with an appVersion change? Regarding your point with the difference between the artifact and the commited chart to git: I could change the CI to delete a wrong tag and (force) push a new with the correct versions, but I don't really like the idea of automating this because it'd require a push to main and an automatically generated git tag, which would be the wrong direction IMO.

These problems might even require the bug label again since our current chart versioning does something essentially wrong.

@puffitos
Copy link
Collaborator

To be honest, by being mindful and disciplined. We should set up a clear process on how releases are made. We don't release every other day, we still have to write our release notes, we Tag our release branch manually.

Some things are manual. I personally am fine with that.

To avoid errors, PRs should be small in size. Take a look in the various PRs we've opened the last couple of months.

Checks should be in place, to catch errors that happen. I'm pretty confident we can write a pre-push hook that just runs the commands you've added to ci.

The maintainers of this repo should all have pre-commit installed. If they don't, then we should work on that. This is human error that can and should be fixed by human interaction.

@y-eight @niklastreml feel free to chime in. If you all feel the same, then move on like this 👍 it's a valid approach to try and automate things that cause errors, but IMHO it's an over optimization. This adds complexity to the ci that we do not need.

* ci: adjust helm chart appVersion before creating release
* chore: update chart maintainers & bump appVersion

Signed-off-by: lvlcn-t <[email protected]>
Signed-off-by: lvlcn-t <[email protected]>
…hart.yaml

* refactor: add Chart.yaml template with Makefile for automatically adding version and appVersion
* docs: auto regenerate chart README.md
* chore: adjust workflow to use new chart template
* chore: adjust pre-commit hook to use new make docs target

Signed-off-by: lvlcn-t <[email protected]>
Signed-off-by: lvlcn-t <[email protected]>
* chore: rename Chart.yaml template to Chart.template.yaml for clarity
* feat: add make gen target to generate Chart.yaml from Chart.template.yaml
* chore: add Chart.yaml to .gitignore

Signed-off-by: lvlcn-t <[email protected]>
@lvlcn-t lvlcn-t force-pushed the fix/chart-appVersion branch from a4375b1 to a08c1f0 Compare November 19, 2024 13:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci Issues/PRs related to github actions area/helm Issues/PRs related to the helm chart housekeeping
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants