-
Notifications
You must be signed in to change notification settings - Fork 324
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
More consistent helm package versions #1251
Closed
TBBle
wants to merge
8
commits into
googleforgames:master
from
TBBle:more_consistent_helm_package_versions
Closed
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
d87cc6a
Update Helm deps for telemetry subchart as well
35ee147
Generate YAML from Helm package, not the source
33c9c61
Set package AppVersion to the container image tag
96a91b6
Remove needless dependency of install-demo on Helm
256a831
Patch global image tag value before packaging
211c798
Update chart deps before packaging the chart
955ffda
Use the CI OCI Registry in non-release Helm charts
d86db29
Include Helm chart and evaulator YAML in artifacts
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
values.yaml is commited, so running any command which calls into this would cause unwanted churn on these file, right?
Why are you replacing the current helm_image_tags?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because right now, the generated chart contains floating image tags (0.0.0-dev),
and I don't think(I checked, it's set by default in values.yaml... A different code-smell.)imagePullPolicy: Alwaysis set here.So installing this chart will mean your pods are running whatever version of the image happened to be built from master at the time of
first pull for that nodepod startup. Over time, this means you'll end up with a mix of different versions of the same image, which seems a huge risk.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Patching the registry is needed because the auto-build images are not pushed to
gcr.io/$(OPEN_MATCH_PUBLIC_IMAGES_PROJECT_ID)(which is what's in the committed values.yaml), but togcr.io/$(OPEN_MATCH_BUILD_PROJECT_ID).The idea here is to test the same thing we ship. Before this change, the behaviour was to generate templates from the Helm chart, and patch those generated files for CI, leaving the Helm chart untested and invalid out-of-the-box.
Another option would be to make the helm calls for CI, and the users of the 0.0.0-dev chart, pass
--set global.image.registry=gcr.io/$(OPEN_MATCH_BUILD_PROJECT_ID)to Helm on each invocation (or when using it as a subchart, hard-coding that in the parent chart's values.yaml).A different solution would be to have the Helm chart template its registry value to detect a GIT SHA as the image tag, and in that case replace
gcr.io/$(OPEN_MATCH_PUBLIC_IMAGES_PROJECT_ID)withgcr.io/$(OPEN_MATCH_BUILD_PROJECT_ID)if the user has not overridden the registry directly.I feel like that would be less clear than having the same script that pushes the images also tell the Helm package where it pushed those images to, but it would mean the Helm chart is packaged from the unadulterated source, which is nice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, the change made by this target should not be committed, since it's patching in build-specific values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I don't want commands making changes to files which shouldn't be committed (exception: anything in .gitignore, obviously). At best it'd be annoying to revert, especially when there's other changes to the same file, at worst changes accidentally make there way into being committed.
Why is simply setting the variables when building the charts not the right course of action here?
Sorry for the delay reviewing this - I wasn't involved in writing the charts nor the build system so I need time to become familiar and figure out the best solution here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't set variables except
.Chart.Versionand.Chart.AppVersionwhen callinghelm package. The best you can do is change the values.yaml before callinghelm package. That's why I suggested as a TODO to move the image tag be.Chart.AppVersion, because we can set that at package-build time, to remove 50% of this editing.To get rid of the other 50% of the editing, we'd need to instead have a macro reacting to
.Chart.AppVersionor.Chart.Version, and recognising an auto-build, to replace the existing hard-coded default repo with two hard-coded default repos, selected by, e.g. the presence of-devin.Chart.Version.However, both of the above changes are complicated by the current muliti-chart setup, as the sub-charts always have version
0.0.0-dev, even in release builds, because we don't usehelm packagefor them, but source them inline, which means there's no opportunity to set.Chart.Versionand.Chart.AppVersionat package time.We could, as a separate change, modify the build system to do
helm packagefor the subcharts rather thanhelm dep updateon the parent charts, perhaps. But per #1244, the whole subcharting setup in Open Match is pretty messy, and I'd rather spent the time unwinding that, if I was going to spend time on it. (And any Open Match time I spend has higher priorities, like the outstanding documentation updates for #1235).Perhaps with some clever scripting, the values.yaml changes could be reverted as part of the Make execution? Although obviously that won't fly if the process fails and the user just runs it again, because then on next run, it'd be changing from the modified files anyway, and change them back to the modified files.
To be fair, this is a pretty common problem I've hit with container image repositories and registries, if you have separate 'release' and 'auto-build' output locations. There was a proposal for Helm to make image-registry handling more formalised and allow solving issues like this more streamlined, but it didn't seem to go anywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(╯°□°)╯︵ ┻━┻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was briefly a feature to do exactly that, but it broke other workflows, e.g., stripped all the comments from the values.yaml, and was backed out. The feature might come back in the future, but it's been a slow, 2-year burn so far. So I'm not personally building any CI pipelines around the assumption I'll have it in a meaningful timeframe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was thinking about this more, would it make sense to switch to "building" charts?
Basically:
This would also get rid of committed zip file sub charts (which kind of also shows that there are build steps to the charts already, they're just a bit of a hack today.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, that would definitely be an option. And if you're doing that, I'd just copy the current subcharts into the charts/ folder, rather than building them separately and copying the tarballs in, since (as far as I recall) they are not deployed separately anywhere anyway. In effect, Helm's just extracting those those tarballs internally and treating the whole thing as one big directory anyway, so it removes a needless step of "building" the subcharts, when they don't really have an independent life outside the open-match chart.
You'd still run
helm dep uporhelm dep build(I forget which one is in the OM workflow) to update external charts, but that won't touch chart directories in the charts/ folder.