Discussion: Helm chart "version" and "appVersion"

[krisztiankoszegi]

Hi, it would be nice if the Chart.yaml file contain the appVersion to separate the helm chart version from the actual-runner-controller version.

I would suggest keeping them different, to follow Helm documentation.

Updating version in the helm chart every time actions-runner-controller got a new release doesn't make much sense, when the helm chart (version) has not been changed, but the application version it deploys has been changed (appVersion).

What do you think?

[timcosta]

I was about to open an issue about this as well, but with a slightly different suggestion. I think it would be great if the helm chart version matched the version of actions-runner-controller that it deployed, because as of right now the chart version is 0.10.2 which deploys 0.17.0, but that feels very non-obvious to me. I had to go look up https://summerwind.github.io/actions-runner-controller/index.yaml to see what version of helm chart was latest/available, but if the version matched then it would be a lot less guesswork.

[callum-tait-pbx]

This has been discussed various times:
#302
#296
#343

If the community wants the appVersion back then I guess the release workflow needs to be updated to include the app version within the release name:
actions-runner-controller-0.10.2 becomes actions-runner-controller-0.10.2-0.17.0 so we can do multiple formal releases against the same chart version.

I'm of the view the appVersion in the chart is misleading and not very helpful for this specific project however if the community and the maintainer feel it is useful then by all means add it back in and patch the release process.

[mumoshu]

@timcosta Hey! I hear you but -

I think it would be great if the helm chart version matched the version of actions-runner-controller that it deployed

This isn't how a standard Helm chart works, AFAIK.

@krisztiankoszegi Hey! So there's a historical reason as @callum-tait-pbx has kindly explained.

The primary reason I wanted to avoid appVersion was that I had no reliable automation to keep appVersion up-to-date. If anyone contributes it, I'm fine to bring it back.

Also, to clarify -

actions-runner-controller-0.10.2 becomes actions-runner-controller-0.10.2-0.17.0 so we can do multiple formal releases against the same chart version.

I would just keep using actions-runner-controller-0.10.2 style versioning. But let the automation bumps the chart number even for an appVersion bump. So that 0.10.2 could have appVersion: 0.17.0 and 0.11.0 could have appVersion: 0.18.0, which removes the "guesswork".

WDYT, everyone?

[callum-tait-pbx]

WDYT, everyone?

You know my view at this point, I think it is more confusing than helpful for this project however if the majority want to maintain an appVersion within the chart then I'm happy to just follow along!

I would just keep using actions-runner-controller-0.10.2 style versioning. But let the automation bumps the chart number even for an appVersion bump. So that 0.10.2 could have appVersion: 0.17.0 and 0.11.0 could have appVersion: 0.18.0, which removes the "guesswork".

Would the actions-chart-releaser work with that? We have farmed all the leg work out to the helm/chart-releaser-action action for Helm releases and I would think we would get the error Error: error creating GitHub release: POST https://api.github.com/repos/summerwind/actions-runner-controller/releases: 422 Validation Failed [{Resource:Release Field:tag_name Code:already_exists Message:}] as our release name actions-runner-controller-0.10.2 would not unique between specified app versions? I think if we are going to publish mutiple charts of the same chart version but several app versions then the workflow would need reengineering to allow for appending to an existing release or we need to use the --release-name-template flag to provide a unique github release name for every release e.g. actions-runner-controller-0.10.2-0.17.0? Might be wrong on that though! That's just how it looked to me just having a glance!

I see what you mean, this opens up lots of confusion to me and breaks the chart verison being a semantic versioning number which I'm not a fan of. Maintaining semantic version of the chart is especially important with this project as we know we will have to release a breaking chart for cert-manager API changes and k8s API changes, even more so because of a lack of release notes for Helm releases. The only piece of information we will have atm to indicate what type of changes are being deployed is in that semantic versioning of the chart. The only real way of including the app version in the Helm chart is by changing the github release name used with the --release-name-template flag and adopting:

• actions-runner-controller-0.10.2-0.17.0
• %CHART_NAME%-%CHART_VERISON%-%APP_VERSION%

EDIT I suppose the main issue with your proposed method is that realistically we won't bump the chart version correctly some of the time. Sometimes the manager updates will amount to bug fixes and so we should bump the patch, sometimes backwards compatible features and we should bump the minor etc. So I guess the 3 options are:

• Just let automation always bump a specific verison number e.g. minor and accept we are breaking the semantic versioning of the chart version
• Best endeavours of manually bumping the chart version correctly and accept it will be done wrong sometimes and not worry too much about that beyond ensuring we are bumping the major version correctly as those are the releases that can break existing deployments. We've historically released charts without bumping correctly or forgetting to bump the appVerison value so it is likely this will happen from time to time.
• Make the release of the chart more formal and have the workflow release the chart on publish rather than just whenever there is merge effecting anything in the charts folder structure, the maintainers will be more aware of what changes happened to the app between releases and are more likely to follow the process. We've historically had chart changes be merged without the chart version bumped or bumped incorrectly so this would alleviate that but be more work for the maintainers.

Pick your poison I guess! 🤷
Again, happy to go along with what the majority want though!

[callum-tait-pbx]

Made a PR to add it back with a contributing.md file, feel free to merge @mumoshu if you want the appVersion included going forward

[mumoshu]

@callum-tait-pbx Thanks for the comment and the PR! I'll try to check them asap.

[mumoshu]

@callum-tait-pbx Hey! Thanks for your detailed feedback. It has been and will keep being super helpful for me to move this project forward in a productive and healthy manner.

I may have missed/misread some part of your comment but I'll do my best to reply every important part of it.

I see what you mean, this opens up lots of confusion to me and breaks the chart verison being a semantic versioning number which I'm not a fan of

I think I had the same sentiment as yours before. Then I thougut just bumping the helm chart minor version when the controller minor version increases would just do it, assuming you can still see that the chart has some "minor" change. The helm chart minor version can change due to some minor change in the chart itself, or minor change in the controller.

The only piece of information we will have atm to indicate what type of changes are being deployed is in that semantic versioning of the chart

I believe this is absolutely a problem.

Sometimes the manager updates will amount to bug fixes and so we should bump the patch, sometimes backwards compatible features and we should bump the minor etc

Ack

Just let automation always bump a specific verison number e.g. minor and accept we are breaking the semantic versioning of the chart version

I think we don't necessarily break the semantic versioning of the chart. Again, we can bump the helm chart minor version when some minor change is made to the chart itself, or some minor change is made in the controller. Similarly, we can bump the helm chart patch version on either a bug fix in the chart or the controller.

Best endeavours of manually bumping the chart version correctly and accept it will be done wrong sometimes and not worry too much about that beyond ensuring we are bumping the major version correctly as those are the releases that can break existing deployments. We've historically released charts without bumping correctly or forgetting to bump the appVerison value so it is likely this will happen from time to time.

Ultimately wrong versioning can happen due to human mistake. There might be the case we manually bump chart version incorrectly or controller chart version. Even if we reduced the chance of mistakes by e.g. automating chart version bump on new controller release, the source of those errors are human mistakes so I think that's not 100% reliable. I think the only way would be to we accept that incompleteness.

Make the release of the chart more formal and have the workflow release the chart on publish rather than just whenever there is merge effecting anything in the charts folder structure, the maintainers will be more aware of what changes happened to the app between releases and are more likely to follow the process. We've historically had chart changes be merged without the chart version bumped or bumped incorrectly so this would alleviate that but be more work for the maintainers.

This is definitely something I'd like to investigate further.

At least, as similar as @int128 is trying in #416, we could potentially use renovate or our own solution to automatically submit a pull request against our chart for version bump on each new controller release. Then the only remaining task is to decide which part of the version to bump, major, minor, or patch.

It can be pretty hard to fully automate it. But at least we can build our own tool to force the operator(me) to manually review changes in the chart or the controller before bumping chart version, so that the operator would have less chance doing it terribly wrong :)

[callum-tait-pbx]

In aid of returning the appVersion back we could implement the work done helm/chart-releaser-action#8 (comment) here to replace the Helm action. The main issue with the process atm is the fact that the helm chart action wants to define the tag / release. If we implement the orbit pipeline then chart releases can just become a part of the steps for the release workflow. The fact the action wants to control the tag is a super weird design decision and the community agrees:

helm/chart-releaser-action#8
helm/chart-releaser-action#70

EDIT

https://github.com/stefanprodan/helm-gh-pages in fact this looks like what we would want, packages your charts, patches the index.yaml and publishes it to gh-pages without trying to manage the release / tag aspects. This could easily be integrated into the release workflow and we could start doing more formal Helm chart releases in conjuction with the manage app.

EDIT

@mumoshu I've been thinking about this and I think the direction we should go is to pivot to this repo being a Helm repository first and foremost, reasons:

• You wouldn't deploy this project without some sort of tool i.e. Kustomize or Helm
• Helm is the more popular tool. Since I've been contributing to this repository I don't think we've had a single issue raised for the Kustomize deployment. I have no view on which is better but I don't think that is important really, I think it is clear that Helm is the more popular tool and so this is where the demand for support / enhancements will be
• Releases can be much cleaner when targeting Helm specifically rather than targeting the app primarily and then Helm releases bolted on as an after thought

We can continue attached the raw YAML to releases as well as keeping the kustomize stuff and adding to it when needed but the key thing is that we move to only doing 1 type of GitHub release, that release is a Helm chart. The main pain point as it stands is the fact that we have 2 unique types of releases.

Thoughts?

[callum-tait-pbx]

@mumoshu I'm really keen to hear your thoughts on this and if you are happy, to start fleshing out the change. I think moving Github releases to being Helm chart releases exclusively would massively clean up the release process without negatively impacting anyone consuming the solution through either of the other means. We would still attach the raw yaml to the release for those that are deploying the solution that way just as other Helm repos do e.g. https://github.com/jetstack/cert-manager/releases.

[mumoshu]

think moving Github releases to being Helm chart releases exclusively would massively clean up the release process without negatively impacting anyone consuming the solution through either of the other means.

@callum-tait-pbx Hey! I think I generally agree with your idea, but the solution I had in my mind was something opposite- create a dedicated GitHub repository containing Helm charts and releases.

That way, we can keep using this repository's GitHub Releases for the controller and the manifests tied to the controller.

It should also be important to keep having the unreleased, development version of the chart INSIDE this repo, to be pushed and released via/in the GitHub repository dedicated to the chart.

[mumoshu]

I've just created a GitHub organization and a repository dedicated for hosting released charts' sources and versions: https://github.com/actions-runner-controller/charts

My intuition says that it should be a matter of just moving https://github.com/summerwind/actions-runner-controller/blob/master/.github/workflows/on-push-master-publish-chart.yml to the new repo, and updating the workflow in this repo to only git-push the latest helm chart to the main branch of https://github.com/actions-runner-controller/charts.

WDYT?

[toast-gear]

Discussing internally how to release charts going forward, we will most likely be moving the Helm implementation to https://github.com/actions-runner-controller/charts