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
chore: add helm lint #2174
chore: add helm lint #2174
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2174 +/- ##
==========================================
+ Coverage 64.19% 64.51% +0.32%
==========================================
Files 107 111 +4
Lines 14924 15542 +618
==========================================
+ Hits 9580 10027 +447
- Misses 4768 4899 +131
- Partials 576 616 +40 ☔ View full report in Codecov by Sentry. |
tools/make/helm.mk
Outdated
@@ -27,3 +27,7 @@ helm-install: helm-generate ## Install envoy gateway helm chart from OCI registr | |||
|
|||
helm-generate: | |||
ImageRepository=${IMAGE} ImageTag=${TAG} envsubst < charts/gateway-helm/values.tmpl.yaml > ./charts/gateway-helm/values.yaml | |||
|
|||
lint: helm-lint | |||
helm-lint: helm-generate |
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.
looking at our CI
https://github.com/envoyproxy/gateway/blob/main/.github/workflows/build_and_test.yaml
it looks like helm-generate
is directly called
should we just move the helm lint
command as a step inside helm-generate
so CI can catch this ?
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.
According to your recommendation, it is completely okay in ci. but I think helm lint
should be a part of make lint
Otherwise, if the user executes make lint
separately, there will be nohelm lint
step.
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.
yeah instead it will get triggered in helm generate
step and fail that step which imo is fine.
If we include helm lint
in lint
we will end up calling helm generate
multiple times
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.
make sense. Already updated.
Signed-off-by: Song <uxiaosongsong@gmail.com>
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.
LGTM thanks !
What type of PR is this?
chore: add
helm lint
atmake lint
stage.What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes 2057