-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Return cluster-autoscaler-chart Chart name to cluster-autoscaler #3679
Conversation
Yeah, that looks like it will do the trick. Many things in _helpers.tpl are driven off .Chart.Name, and so long as that is set the same as it was before migration from the legacy to new repo location, all the resources should not go through a destroy/create cycle. Thx very much! |
charts/cluster-autoscaler/Chart.yaml
Outdated
sources: | ||
- https://github.com/kubernetes/autoscaler/tree/master/cluster-autoscaler | ||
type: application | ||
version: 1.1.1 | ||
version: 2.0.0 |
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.
Hmm.. unclear if this should be 9.0.0 or something, tho. Since the last version was 8.0.0 before the chart took a detour through the other name.
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, that's a good point and would probably make migration from the deprecated chart easier on users.
I'll update this to 9.0.0 and update the docs to make clear why this is happening as well.
@gjtempleton you could modify you checkout step to look like the following so you don't need a seperate fetch step. - name: Checkout
uses: actions/checkout@v2
with:
fetch-depth: 0 Also the example has a additional install helm step that may be required? |
I can confirm that the additional helm step is required. |
eef4c6e
to
10cfef4
Compare
Update helm/chart-releaser action to achieve this
10cfef4
to
e6c42be
Compare
Is there a roadmap for when this might be released? |
@stevehipwell I can't commit to a timeline as it'll need approval from one of the other repo owners /assign @MaciekPytel |
Thx for the update. Our upgrades to k8s 1.18 are sort of on hold until this gets merged. |
Thank you @gjtempleton ! |
still waiting on this |
The parallel kubernetes-sigs/descheduler#436 was merged yesterday if that helps? |
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
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gjtempleton, llamahunter, mwielgus The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Fixes #3475
Update helm/chart-releaser action to leverage new functionality of the chart releaser action allowing custom overrides of the published chart artifact separate from the naming of the chart
Tested successfully on my fork here: https://github.com/gjtempleton/autoscaler/releases/tag/cluster-autoscaler-chart-2.0.0
This will keep the names of the build artifacts consistent with the existing ones for the 1.X.X releases of the chart, but update the naming in the published chart to be neater and consistent with best practices.
I've also included a note in the readme to warn users.