Skip to content
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

Rename Helm chart #436

Merged
merged 1 commit into from
Nov 18, 2020
Merged

Rename Helm chart #436

merged 1 commit into from
Nov 18, 2020

Conversation

stevehipwell
Copy link
Contributor

Now that the helm/chart-releaser and helm/chart-releaser-action can support creating a custom tag name for helm chart release the descheduler helm chart no longer needs to be named to avoid tag collisions.

This PR updates the CI tooling for helm, renames the descheduler helm chart to descheduler from descheduler-helm-chart and updates the related documentation.

On applying this merge request the existing helm charts will not be modified or removed and only future releases will be impacted. My suggestion would be to use the helm/chart-releaser with the --commit flag to re-create the existing charts with the new name in parallel.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 9, 2020
@k8s-ci-robot
Copy link
Contributor

Hi @stevehipwell. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 9, 2020
@stevehipwell
Copy link
Contributor Author

@seanmalloy is this something you could look at?

@stevehipwell
Copy link
Contributor Author

@ingvagabund @k82cn could you let me know if there is anything else you need to approve this for testing?

@ingvagabund
Copy link
Contributor

@stevehipwell appologies, I have no knowledge of helm. @damemi and @seanmalloy were participating in the effort.

Copy link
Contributor

@damemi damemi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/ok-to-test
Thanks for updating us on this @stevehipwell! I'm curious though if you could point me to the PR that enables the CR_RELEASE_NAME_TEMPLATE env var in the chart-releaser action. From the v1.1.0 release notes you linked (or anywhere in the repo) I didn't see anything that stood out to me as enabling it and I found this issue and PR that are still open to add it (helm/chart-releaser-action#37 & helm/chart-releaser-action#57)

I do see some other changes that look helpful, like helm/chart-releaser-action#42 which will tag the branch the release is built from instead of the main branch (I think this will get our automated builds back to just being tagged descheduler)

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 13, 2020
@stevehipwell
Copy link
Contributor Author

@damemi I think I missed the fact that the helm/chart-releaser-action could provide arguments to the helm/chart-releaser as environment variables rather than just as CLI arguments. I went through the code again after the v1.1.0 release and figured out how it should work.

As far as helm/chart-releaser-action#37 goes I'll close it once this is proven to work.

@stevehipwell
Copy link
Contributor Author

@ingvagabund apologies, I missed off @damemi from my original comment and then when @seanmalloy didn't reply I saw that you and @k82cn were listed as the reviewers so tried you.

@damemi
Copy link
Contributor

damemi commented Nov 13, 2020

@stevehipwell ah that makes sense, thanks for clarifying. In that case I think this looks good here. I'll let @seanmalloy lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: damemi, stevehipwell

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 13, 2020
@seanmalloy
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 18, 2020
@k8s-ci-robot k8s-ci-robot merged commit 5225ec4 into kubernetes-sigs:master Nov 18, 2020
@stevehipwell stevehipwell deleted the helm-chart-rename branch November 18, 2020 08:42
@stevehipwell
Copy link
Contributor Author

@seanmalloy I could have a go at the manual chart creation and create a PR to add the correctly named charts to the gh-pages branch if that would be helpful?

@seanmalloy
Copy link
Member

@seanmalloy I could have a go at the manual chart creation and create a PR to add the correctly named charts to the gh-pages branch if that would be helpful?

@stevehipwell sounds good to me.

@stevehipwell
Copy link
Contributor Author

@seanmalloy I've just looked into this further and realised that someone will need to do it on this repo; I've documented the steps below as best as I can figure out. These steps assume, correctly I think, that the current chart is backwards compatible with all existing releases.

The setup steps are below.

  • Install the helm/chart-releaser and add to path
  • Navigate to repo root and make sure the branch is set to master
  • Get GitHub auth token and export it as CR_GH_TOKEN

I think we want to release the following charts; one for each release with a Docker image.

version: 0.19.1
appVersion: 0.19.0
version: 0.18.2
appVersion: 0.18.0
version: 0.10.0
appVersion: 0.10.0

For each block above follow these steps to package the charts.

  • Modify ./charts/descheduler/Chart.yaml with the content of the block
  • Run cr package ./charts/descheduler

The following steps should be completed once all the charts have been packaged.

  • Run cr upload --token "${CR_GH_TOKEN}" --release-name-template "descheduler-helm-chart-{{ .Version }}" --owner kubernetes-sigs --git-repo descheduler
  • Run cr index --token "${CR_GH_TOKEN}" --release-name-template "descheduler-helm-chart-{{ .Version }}" --owner kubernetes-sigs --git-repo descheduler --charts-repo https://kubernetes-sigs.github.io/descheduler

As an aside I'd also suggest removing everything from the gh-pages branch that isn't index.yaml.

@seanmalloy
Copy link
Member

@stevehipwell thanks for the info. Between myself @and @damemi we will sort this out.

@damemi
Copy link
Contributor

damemi commented Nov 20, 2020

Do we need to manually merge to the gh-pages branch? We run the chart-releaser action when any PRs merge against the release-* branches, so could it be done that way?

@stevehipwell
Copy link
Contributor Author

@damemi the steps I've outlined above should be correct to add renamed releases without needing any manual commits. Moving forwards, the same process as previously should work.

@stevehipwell
Copy link
Contributor Author

@seanmalloy & @damemi it looks like it would be best practice to deprecate the descheduler-helm-chart releases when creating the new descheduler releases as suggested by kubernetes/autoscaler#3475 (comment). Based on #444 (comment) my suggestion would be to add the following block to the ones I put above for the manual process to release a deprecated version. This would mean using a chart version 0.19.2 instead of 0.19.1 for for the actual v0.19.0 chart re-release as the repo tags need to be unique for each release.

name: descheduler-helm-chart
version: 0.19.1
appVersion: 0.19.0
maintainers: []
deprecated: true

@damemi
Copy link
Contributor

damemi commented Dec 8, 2020

Coming back to this, I'm not totally clear on the difference between running the steps listed above (#436 (comment)) vs backporting the changes from this PR to the release branches.

@stevehipwell I understand that running the chart-releaser tool locally removes the need for making manual changes, but I think it would actually be ideal if this could be done through the regular PR workflow for history tracking. If it's possible we should try not to push changes to this repo without the usual review/approval steps.

Could you clarify some more if I'm misunderstanding a necessary step in your process that wouldn't be covered by backporting these changes to the release branches in github? (And thank you for your help in leading our helm changes 🙂)

@stevehipwell
Copy link
Contributor Author

@damemi you should be able to do this via backporting. Each release would need two PRs, one to deprecate the old chart and a second to add the new chart. Would you like me to open these for both 1.18 and 1.19? If it works I could open a single PR to create a chart for 1.17.

@stevehipwell
Copy link
Contributor Author

@damemi on a slightly separate note is anyone in this team part of the https://artifacthub.io/ kubernetes-sig organisation? If so you should be able to move the organisation ownership of this repo to the organisation; RE kubernetes-sigs/metrics-server#572 (comment).

@damemi
Copy link
Contributor

damemi commented Dec 8, 2020

@stevehipwell if it's not too much work for you, that would be very helpful! And with 1.20 coming soon, we will only need to support back to 1.18 (so I don't think creating a 1.17 chart is necessary, if I'm wrong though feel free to create that PR as well)

For the artifacthub org, I don't think any of us are part of it. We can track that in a new issue, because that sounds like a good idea to get set up. Thanks!

@stevehipwell
Copy link
Contributor Author

I'll create the MRs, I suspect that you do want a 1.17 version as if you look at the cloud providers supported versions 1.17 isn't going away any time soon. I'll flag the PRs as draft, who should I assign them to once they're all done?

RE artefact hub, it might be worth messaging @scottrigby on that metrics-server thread to get some of you added to the organisation. Currently this repo is part of the Helm catch all organisation which I think is from the migration from the original helm hub.

@damemi
Copy link
Contributor

damemi commented Dec 8, 2020

Feel free to assign them to me, and I'll approve (and make sure the chart-releaser github action runs).

We'll look more into the artifacthub org, thanks for linking to someone we can talk to over there

@stevehipwell
Copy link
Contributor Author

@damemi there are now 3 PRs that need to be released in order to rename the charts, I'm just about to assign you and remove the draft flag.

PR #458
PR #457
PR #459

After this has been done you might want to backport the latest chart changes onto the release branches and create a v0.17 chart release.

@damemi
Copy link
Contributor

damemi commented Dec 8, 2020

Thanks @stevehipwell! I'll review those PRs and if come to you with any questions :)

briend pushed a commit to briend/descheduler that referenced this pull request Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants