-
Notifications
You must be signed in to change notification settings - Fork 419
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
Pin local chart dependencies in Chart.yaml files #1192
Pin local chart dependencies in Chart.yaml files #1192
Conversation
Hey @keithhand thanks for this PR. As I recall, the "requirements" field in the Chart.yaml may break some third-party integrations including the rancher partner-charts. It's getting time for an upgrade there anyway, maybe @elenalape can hand that off to the support engineering team, then we can
|
@keithhand following up here now that you've had a chance to look at the rancher process. Does this pr interfere with that? |
@AjayTripathy No, this should go through fine. I checked a few projects from the partner-charts repo, and this formatting seems acceptable since a few other projects are using it. I didn't add condition fields for each dependency, which doesn't seem necessary for the merge, but it isn't a bad idea. I can push that up after making sure things configure correctly. |
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.
Other than my question to Ajay, this LGTM.
dependencies: | ||
- condition: kube-state-metrics.disabled | ||
name: kube-state-metrics | ||
repository: file://./charts/kube-state-metrics |
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.
@AjayTripathy After our metrics emission changes, do we still need to have KSM in the Helm chart?
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.
Probably not but that's a pretty big change outside the scope of this PR.
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.
Still LGTM!
- condition: global.grafana.enabled | ||
name: grafana | ||
repository: file://./charts/grafana |
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.
cc @nealormsbee for your (eventual) Grafana deprecation/removal
What does this PR change?
Pins local chart dependencies inside helm
Chart.yaml
filesDoes this PR rely on any other PRs?
N/A
How does this PR impact users? (This is the kind of thing that goes in release notes!)
Allows
helm lint
command to pass.Links to Issues or ZD tickets this PR addresses or fixes
How was this PR tested?
After successfully upgrading the helm chart I was able to also successfully run
helm lint
.Have you made an update to documentation?
N/A