Host stable/kube-state-metrics helm chart #1237
Conversation
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
the
do-not-merge/invalid-owners-file
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. I understand the commands that are listed here. |
k8s-ci-robot
added
the
cncf-cla: no
|
I believe the CLA problem is because I'm (purposefully) also importing other user's commits from the stable repo. |
k8s-ci-robot
added
size/XXL
charts/kube-state-metrics/Chart.yaml
Outdated
| - monitoring | ||
| - prometheus | ||
| - kubernetes | ||
| version: 2.9.2 |
There was a problem hiding this comment.
I suggest to bump version to 2.10.0. That allows to use 2.9.2 in the old repo to deprecate the chart over there.
There was a problem hiding this comment.
@torstenwalter 👋 Good question, I had to think about this for a moment.
Re semver, this chart repo move isn't a backwards incompatible change (MAJOR), or a new feature (MINOR), so PATCH is fine.
In general when charts move repos they don't bump two PATCH versions to address a conflict with the deprecation bump in stable. We could do that here, but it give us anything – there is no version conflict since the release will be from a different repo.
If you see a problem with this line of thinking lmk. But I wouldn't think so since this is the way every other chart repo move has done it.
| - prometheus | ||
| - kubernetes | ||
| version: 2.9.2 | ||
| appVersion: 1.9.7 |
There was a problem hiding this comment.
We launched a pre-release candidate v2.0.0-alpha which is now merged in master, if we merge this it will differ.
There was a problem hiding this comment.
Hi @lilic. In general it's ok for the appVersion in a chart to be behind the newest release of the app it installs (for a short time anyway, it has to be, since the container image tag for the app must be available for CI to pass for the PR.
But in this case, 1.9.7 is the most recent release, right? https://github.com/kubernetes/kube-state-metrics/releases/tag/v1.9.7
There was a problem hiding this comment.
We have a pre-release of v2.0.0-alpha cut a few days ago, not sure if that counts for helm charts and the meaning of stable in helm? Just for your information https://github.com/kubernetes/kube-state-metrics/releases/tag/v2.0.0-alpha. (no objections to using 1.9.7 at all, just want folks to switch to 2.0 as soon as possible :) )
|
@brancz @lilic @tariq1890 any feedback on this PR. |
|
@scottrigby sorry I was under the impression is not ready for review yet. I have not used helm recently but will give it a try in reviewing. But the biggest problem right now is the CLAs that were not signed and we cannot merge without those signed:
|
|
@lilic Hi 👋 I'm happy to move this out of draft mode, I was just looking for feedback per #1153 (comment) and #1237 (comment). Re CLA, from what I understand CNCF requires either CLA or DCO. Most of these commits have DCO signoff, and for those which don't (since many of these commits were made before DCO was enforced) I could add mine. However I understand Kubernetes contribution guidelines require CLA. I could not hope to get that from all of these contributors, so can we discuss allowable options for retaining this commit history? I believe history is important for several reasons - both credit to those who worked on it as well as a record of past change motivations that may help inform future decisions (especially if we hope to simplify the chart in future due to new options such as Helm's post-renderer flag). What do you think about me modifying the commits to list myself as author and crediting the original committers as co-authors? I could add a note in the commit message clarifying that modification was made due to CLA requirements, and this way it would still be an accurate depiction of the work, including me collating and brining in the previous work. I would think this would be more acceptable from an attribution point of view than flattening it all and bringing it in as if it were my own commit. If done this way, could my own CLA then suffice to allow retaining commit history? |
|
Maybe @mrbobbytables or @nikhita have suggestions for importing commits from folks that haven't signed the CLA. |
|
We've merged one-offs like these (CLA problems due to code migration - kubernetes/kube-openapi#211 (comment)) in the past so we could merge it manually. Before we do that, we need to ensure:
@swinslow can you please let us know what would be the right workflow for handling the license/copyright? fyi @dims |
|
Let me know if there is anything you need from my side, I have admin access to merge if we get an okay from Kubernetes org. |
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
@nikhita Great news, yes that would be much easier. I have moved this PR from draft to ready for review. I have also updated the PR description checklist now that some of the steps are no longer needed. |
| # Default values for kube-state-metrics. | ||
| prometheusScrape: true | ||
| image: | ||
| repository: quay.io/coreos/kube-state-metrics |
There was a problem hiding this comment.
Any chance we can change this to the gcr image and build a 1.9.7 image there?
There was a problem hiding this comment.
@eddiezane Good idea. Though based on past experience I would prefer that to be a follow-up PR to keep the scope clear in this one. Does that sound reasonable?
There was a problem hiding this comment.
1.9.7 did not have the gcr images yet, but you can do the migration in v2.0.0 to gcr.
There was a problem hiding this comment.
@scottrigby that works for me.
@lilic Since v2.0.0 will have breaking changes I'd like to advocate for the last v1 release to have an official image (with multi arch support) cut. Should I open an issue as a better place to have that discussion?
|
I have added one additional commit. This was the final commit made to the stable chart since I opened this PR, and before the deprecation notice: helm/charts#23968. The only difference now is the version had bumped again (in case users already have See https://github.com/helm/charts/commits/master/stable/kube-state-metrics/Chart.yaml for a list of all changes to this stable chart's |
|
@nikhita - wrapping up one more open question:
From similar efforts in other k8s projects (example https://github.com/kubernetes/ingress-nginx) the Apache 2.0 License at the root of this repo has been seen as compatible with the same Apache 2.0 License at the root of the now deprecated helm/charts repo. BUT to be on the safe side, in case it helps move this PR along, I added the one from helm/charts (with "The Helm Authors" copyright line) to the |
|
@scottrigby I can merge manually as soon as we get an okay from someone from the cncf or Kubernetes org that it's legally alright to do so, we had to get this okay in another project we are migrating. Or if you can reach out to the folks who did not sign the CLA to sign it? |
It looks like there are at least 17 people who have not signed the CLA. Getting them to sign the CLA wouldn't be feasible... @swinslow or @caniszczyk can you please take a look and suggest next steps from a legal perspective? More context in #1237 (comment) |
Note we may want to add GPG signing in a follow-up PR. Signed-off-by: Scott Rigby <scott@r6by.com>
scottrigby
force-pushed
the
stable-chart-history
branch
from
905fecc
to
fdd0e4b
Compare
@lilic I can see that, but generally chart repos don't work like this. With the helm/chart-releaser-action, each time a PR is merged into the main branch, a new version of the chart is released. Charts also normally have a different version than the application (charts generally version faster than the app it installs). If you want to change this workflow to have fewer releases, you could do it by having a staging branch and merge into the main branch less frequently – in that case you would also want to adjust the chart-testing options, otherwise each PR would still need to bump the version, even though there would be fewer actual releases, which would confuse users. In general this would be an anti-pattern for charts. Is it OK for the chart to have its own release cadence, based on need/PR?
On this question, @caniszczyk said it's up to us which way we do it:
I brought in commits because it has been useful in other projects to have the history. Here is an example (one of many) where someone has asked for commit history to be restored. This is one I happened to help with: goharbor/harbor-helm#48 From what @nikhita said, the fact that CLA bot isn't passing does not have to be a show stopper, as long as we get the go-ahead from @caniszczyk which we now have. I asked @lilic in Slack for an opinion on this (the value of commit history etc), given the above, who said:
So @tariq1890 just checking with you on this. Do you have an issue keeping the commit history (for the reasons above), or would you prefer I squash it out?
@lilic Sounds good, just pushed that change to remove fiunchinho. Do you want to invite @mrueg to the org? Also @lilic and @tariq1890 I just pushed a change to update the Helm repo GitHub actions to the most recent versions. I noticed the app releases are signed with a GPG key. As of v1.0.0 |
|
@scottrigby I'd say we keep the commit history of the helm charts. It would definitely be unfortunate if we lost a record of all the helm chart contributors and their contributions. Final decision : I am in favour of this. |
|
Follow up PR for chart signing sounds good to me. I believe this PR should solely be intended for chart migration. CI improvements can be done in subsequent PRs. Will let @lilic chime in as well |
|
Looks good to me, I am just wondering does the CLA check checks all the code or just the newest commits? So if we merge this will the next PR fail with CLA or will it pass? Otherwise lgtm 🎉 |
It just checks the commits in the PR so future PRs won't have any problems with the CLA. :) @lilic also please feel free to merge manually once OWNERS is fixed! |
|
Thanks can do! @tariq1890 do we want to just leave you in the OWNERS and once @mrueg is part of k8s org, we can add him after that? So we unblock this PR? |
Quick update on this: My request to become part of the org was approved and I've accepted the invite. kubernetes/org#2378 Thanks again for sponsoring! |
|
/verify-owners |
k8s-ci-robot
removed
the
do-not-merge/invalid-owners-file
|
/lgtm Thanks all! 🎉 |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lilic, scottrigby 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 |
k8s-ci-robot
added
the
approved
|
Merging manually. 🎉 😅 |
|
The build failed:
Latest version appears to be v1.1.0: https://github.com/helm/chart-releaser-action/releases |
|
@devurandom see this comment here, this will be done in a follow PR. #1237 (comment) but you are welcome to submit a PR for this! |
|
Keywords which can automatically close issues and at(@) or hashtag(#) mentions are not allowed in commit messages. The list of commits with invalid commit messages:
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. I understand the commands that are listed here. |
- kubernetes/kube-state-metrics#1237 (comment) - kubernetes/kube-state-metrics#1237 (comment) Signed-off-by: Scott Rigby <scott@r6by.com>
- kubernetes/kube-state-metrics#1237 (comment) - kubernetes/kube-state-metrics#1237 (comment) Signed-off-by: Scott Rigby <scott@r6by.com>
- kubernetes/kube-state-metrics#1237 (comment) - kubernetes/kube-state-metrics#1237 (comment) Signed-off-by: Scott Rigby <scott@r6by.com> Signed-off-by: Thor Anker Kvisgård Lange <tal@netic.dk>
- kubernetes/kube-state-metrics#1237 (comment) - kubernetes/kube-state-metrics#1237 (comment) Signed-off-by: Scott Rigby <scott@r6by.com> Signed-off-by: QuentinBisson <quentin@giantswarm.io>
- kubernetes/kube-state-metrics#1237 (comment) - kubernetes/kube-state-metrics#1237 (comment) Signed-off-by: Scott Rigby <scott@r6by.com>
What this PR does / why we need it:
See #1153
To-do:
After PR merge:
[ ] follow-up PR: import chart version package history, and update gh-pages index.yaml file (so that users can continue to access older chart package versions after theNO LONGER NEEDED now that GitHub has sponsored helm/charts package history. See https://helm.sh/blog/helm-turns-five/ 🎉stablerepo is deprecated and garbage collected)[ ] deprecate stable/kube-state-metricsDONE as part of [stable/incubator] Mark all charts as deprecated helm/charts#24310[ ] close any stable/kube-state-metrics issues, pointing users here (there are currently none)This will be taken care of as part of the helm/charts repo deprecation follow-up.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #1153