Allow helm chart verification in both bazel + make#4723
Merged
jetstack-bot merged 2 commits intocert-manager:masterfrom Jan 10, 2022
Merged
Allow helm chart verification in both bazel + make#4723jetstack-bot merged 2 commits intocert-manager:masterfrom
jetstack-bot merged 2 commits intocert-manager:masterfrom
Conversation
This slightly modifies our existing helm chart verification script to accept the location of a tarball containing the chart we're verifying. That in turn allows us to more generally use the script for both validating the chart we build using make and the chart we build using bazel. Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
Signed-off-by: Ashley Davis <ashley.davis@jetstack.io>
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: SgtCoDFish The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
maelvls
reviewed
Jan 10, 2022
Comment on lines
+8
to
+10
| .PHONY: verify-chart | ||
| verify-chart: bin/cert-manager-$(RELEASE_VERSION).tgz | ||
| ./hack/verify-chart-version.sh $< |
Member
There was a problem hiding this comment.
Probably unrelated to your change: I noticed many "in the future" lines:
$ make -f make/Makefile verify-chart
...
Linting chart 'bin/cert-manager-v1.7.0-alpha.0-67-g72027ce62.tgz' using internal dir 'deploy/charts/cert-manager'
cert-manager/Chart.yaml
tar: cert-manager/Chart.yaml: time stamp 2022-01-10 20:10:55 is 0.29450188 s in the future
cert-manager/values.yaml
tar: cert-manager/values.yaml: time stamp 2022-01-10 20:10:55 is 0.294457475 s in the future
cert-manager/templates/NOTES.txt
tar: cert-manager/templates/NOTES.txt: time stamp 2022-01-10 20:10:55 is 0.294428667 s in the future
cert-manager/templates/_helpers.tpl
tar: cert-manager/templates/_helpers.tpl: time stamp 2022-01-10 20:10:55 is 0.294409669 s in the future
cert-manager/templates/cainjector-deployment.yaml
I guess that's something that isn't to be worried about.
Member
Author
There was a problem hiding this comment.
The verification always runs in a docker container and I believe messages like this are related to docker. Longer term I'd like to remove that dependency on docker for doing this verification 👌
Member
|
I tested both ways (bazel and /lgtm |
30 tasks
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
This slightly modifies our existing helm chart verification script to accept the location of a tarball containing the chart we're verifying.
That in turn allows us to more generally use the script for both validating the chart we build using make and the chart we build using bazel.
It also bumps the version of the docker image we use for verifying to the latest version, whose release notes are here. Verification succeeds for both the make and bazel versions of the chart when run locally using v3.5.0.
Important:
make verify_chartis the command called for chart verification in our presubmits.(NB: I actually suspect we can remove the docker image now and use a locally-downloaded tool, but we can save that for another PR. See this comment on our presubmits for more info about why we use docker here)
/kind feature