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

Migrate helm chart #606

Closed
wants to merge 41 commits into from

Conversation

olemarkus
Copy link

What this PR does / why we need it:

As the stable helm repo is retiring, the metrics-server needs a new home.

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 #572

olemarkus and others added 30 commits October 6, 2020 08:56
* Add initial commit of a metrics-server chart

* Fixes metrics-server without rbac

* Fixed some lint errors

* Add a NOTES.txt file

* makes some imrpovements on how the chart looks like compared to helm create

* follow best practices for RBAC

* adds selector in deployment

* incubator/metrics-server: create ServiceAccount based on serviceAccount.create

* incubator/metrics-server: default pullPolicy to IfNotPresent

* incubator/metrics-server: add maintainer

* incubator/metrics-server: replace maintainer full name with github username in Chart.yaml

* incubator/metrics-server: make namespace configurable where possible

* incubator/metrics-server: grant system:auth-delegator clusterrole and the extension-apiserver-authentication-reader role specifically in the kube-system namespace

* incubator/metrics-server: Replace 'metrics-server' with release fullname

* incubator/metrics-server: Make apiService creation conditional

* Revert "incubator/metrics-server: grant system:auth-delegator clusterrole and the extension-apiserver-authentication-reader role specifically in the kube-system namespace"

This reverts commit b35818cf7d5f03269476a0d5044f6677c0e66b26.

* incubator/metrics-server: put system:auth-delegator crb in kube-system

* incubator/metrics-server: disable creation of apiService on CI runs

* incubator/metrics-server: remove an unecessary RBAC rule

* incubator/metrics-server: warn about not setting apiSerivce.create: true

* incubator/metrics-server: update README.md to reflect values.yaml

* stable/metrics-server: promote chart from incubator to stable
* stable/metrics-server: Add olemarkus to OWNERS

* stable/metrics-server: version bump
* [stable/metrics-server] add additional specs to deployment

* [stable/metrics-server] version bump

* [stable/metrics-server] update readme

* [stable/metrics-server] version bump to 1.0.0

* [stable/metrics-server] remove errant documentation
* stable/metrics-server: Make metrics-server arguments customizable

* stable/metrics-server: Bump version. As this chart is in stable, we might as well start with 1.0.0 now

* stable/metrics-server: Changing version from 1.0.0 to 0.1.0 as per review comment
…rviceAccount resource (#7613)

Signed-off-by: Majid Burney <[email protected]>
Signed-off-by: David J. M. Karlsen <[email protected]>
* [METRICS-SERVER] Typo in README.md

Signed-off-by: Mathias Mumrau Chevalier <[email protected]>

* [stable/metrics-server] bump chart version typo



Signed-off-by: Mathias Mumrau Chevalier <[email protected]>
* Add pod security policy support to metrics server

Signed-off-by: Baron Lenardson <[email protected]>

* Update Chart.yaml

Signed-off-by: David J. M. Karlsen <[email protected]>
…(#11383)

* Added new values options to main README file.

Signed-off-by: Steven Wade <[email protected]>

* Minor bump on metrics server chart version.

Signed-off-by: Steven Wade <[email protected]>

* Removing too many blank lines in metrics server chart values.yaml

Signed-off-by: Steven Wade <[email protected]>

* Adding back a newline to the metrics server chart value.yaml file.

Signed-off-by: Steven Wade <[email protected]>
…etrics server deployment (#12110)

* modified indent for deployment

Signed-off-by: cw-sakamoto <[email protected]>

* add annotations and priorityClassName to metrics-server chart

Signed-off-by: cw-sakamoto <[email protected]>

* modified comment in values.yaml

Signed-off-by: cw-sakamoto <[email protected]>

* change tmpl 'if' to 'with'

Signed-off-by: cw-sakamoto <[email protected]>
* add support for pod disruption budget

Signed-off-by: Jay Howard <[email protected]>

* explicitly support current pod disruption budget options

Signed-off-by: Jay Howard <[email protected]>

* lint: add newline at end of file

Signed-off-by: Jay Howard <[email protected]>
…re mutually exclusive and neither is required (#13296)

Signed-off-by: Jay Howard <[email protected]>
Signed-off-by: ryanhartje <[email protected]>
Signed-off-by: Ryan Hartje <[email protected]>

fixed formatting

Signed-off-by: ryanhartje <[email protected]>
Signed-off-by: Ryan Hartje <[email protected]>

forgot to sign off.

Signed-off-by: ryanhartje <[email protected]>
Signed-off-by: Ryan Hartje <[email protected]>

bump version and append values table in README.md

Signed-off-by: ryanhartje <[email protected]>
Signed-off-by: Ryan Hartje <[email protected]>

Update stable/metrics-server/README.md

Co-Authored-By: Tariq Ibrahim <[email protected]>
Signed-off-by: Ryan Hartje <[email protected]>

Update stable/metrics-server/values.yaml

Co-Authored-By: Tariq Ibrahim <[email protected]>
Signed-off-by: Ryan Hartje <[email protected]>

implementing feedback for metrics-server chart. Bumped chart version, updated metric-server-service template with server.labels value

Signed-off-by: Ryan Hartje <[email protected]>

adding new line at EOF for values in metrics-server chart to pass lint

Signed-off-by: Ryan Hartje <[email protected]>

can't win with these autoformatted spaces at the end of my values file...

Signed-off-by: Ryan Hartje <[email protected]>

[stable/metrics-server] Fix readme typo

Signed-off-by: Ryan Hartje <[email protected]>

Update metrics-server chart to show up in kubectl cluster-info

Signed-off-by: ryanhartje <[email protected]>
Signed-off-by: Ryan Hartje <[email protected]>

fixed formatting

Signed-off-by: ryanhartje <[email protected]>
Signed-off-by: Ryan Hartje <[email protected]>

forgot to sign off.

Signed-off-by: ryanhartje <[email protected]>
Signed-off-by: Ryan Hartje <[email protected]>

bump version and append values table in README.md

Signed-off-by: ryanhartje <[email protected]>
Signed-off-by: Ryan Hartje <[email protected]>

Update stable/metrics-server/README.md

Co-Authored-By: Tariq Ibrahim <[email protected]>
Signed-off-by: Ryan Hartje <[email protected]>

Update stable/metrics-server/values.yaml

Co-Authored-By: Tariq Ibrahim <[email protected]>
Signed-off-by: Ryan Hartje <[email protected]>

implementing feedback for metrics-server chart. Bumped chart version, updated metric-server-service template with server.labels value

Signed-off-by: Ryan Hartje <[email protected]>

adding new line at EOF for values in metrics-server chart to pass lint

Signed-off-by: Ryan Hartje <[email protected]>

can't win with these autoformatted spaces at the end of my values file...

Signed-off-by: Ryan Hartje <[email protected]>

[stable/metrics-server] Fix readme typo

Signed-off-by: Ryan Hartje <[email protected]>

[stable/metrics-server] updating service variable formatting to be consistent

Signed-off-by: Ryan Hartje <[email protected]>

[stable/metrics-server] oof, mismatch for vars between values.yaml and the templates

Signed-off-by: Ryan Hartje <[email protected]>

[stable/metrics-server] reverting service values to camelCase as they should be

Signed-off-by: Ryan Hartje <[email protected]>

[stable/metrics-server] updating template format

Signed-off-by: Ryan Hartje <[email protected]>
Signed-off-by: Pierluigi Lenoci <[email protected]>
…lines (#16685)

* bump metrics-server version

Signed-off-by: Pierluigi Lenoci <[email protected]>

* Whitespace formatting, remove trailing blank lines

Signed-off-by: Pierluigi Lenoci <[email protected]>

* Bump version back

Signed-off-by: Pierluigi Lenoci <[email protected]>
* bump metrics-server version

Signed-off-by: Pierluigi Lenoci <[email protected]>

* Postponed to better times
Signed-off-by: Pierluigi Lenoci <[email protected]>

* Let's try with the new version
Signed-off-by: Pierluigi Lenoci <[email protected]>
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: olemarkus
To complete the pull request process, please assign brancz after the PR has been reviewed.
You can assign the PR to them by writing /assign @brancz in a comment when ready.

The full list of commands accepted by this bot can be found 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 size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. labels Oct 6, 2020
@k8s-ci-robot
Copy link
Contributor

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.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
  • Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: [email protected]

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 k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Oct 6, 2020
@olemarkus olemarkus mentioned this pull request Oct 6, 2020
@k8s-ci-robot
Copy link
Contributor

The following users are mentioned in OWNERS file(s) but are untrusted for the following reasons. One way to make the user trusted is to add them as members of the kubernetes-sigs org. You can then trigger verification by writing /verify-owners in a comment.

  • olemarkus
    • User is not a member of the org. Satisfy at least one of these conditions to make the user trusted.
    • charts/metrics-server/OWNERS

@brancz
Copy link

brancz commented Oct 7, 2020

/check-cla

@brancz
Copy link

brancz commented Oct 7, 2020

I'm unsure how we can accept this without the CLA status being resolved. I'm generally happy with this change, but I think we will need each person to sign the CLA to move forward with this if we want to preserve history.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2020
@k8s-ci-robot
Copy link
Contributor

@olemarkus: PR needs rebase.

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.

uses: helm/[email protected]
with:
command: install
config: ct.yaml

Choose a reason for hiding this comment

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

Needs newline at EOF

Comment on lines +21 to +30
# See https://github.com/helm/chart-releaser-action/issues/6
- name: Install Helm
run: |
curl -fsSLo get_helm.sh https://raw.githubusercontent.com/helm/helm/master/scripts/get-helm-3
chmod 700 get_helm.sh
./get_helm.sh
- name: Add dependency chart repos
run: |
helm repo add stable https://kubernetes-charts.storage.googleapis.com/
helm repo add incubator https://kubernetes-charts-incubator.storage.googleapis.com/

Choose a reason for hiding this comment

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

These steps are not needed, since the metrics-server chart has no dependencies:

Suggested change
# See https://github.com/helm/chart-releaser-action/issues/6
- name: Install Helm
run: |
curl -fsSLo get_helm.sh https://raw.githubusercontent.com/helm/helm/master/scripts/get-helm-3
chmod 700 get_helm.sh
./get_helm.sh
- name: Add dependency chart repos
run: |
helm repo add stable https://kubernetes-charts.storage.googleapis.com/
helm repo add incubator https://kubernetes-charts-incubator.storage.googleapis.com/

- name: Run chart-releaser
uses: helm/[email protected]
env:
CR_TOKEN: "${{ secrets.GITHUB_TOKEN }}"

Choose a reason for hiding this comment

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

Needs newline at EOF.

Copy link
Contributor

Choose a reason for hiding this comment

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

@olemarkus I'd suggest taking a look at the kubernetes-sigs/descheduler release.yaml as it has the updated GitHub action versions and will allow you to use a custom tag for helm charts (with the CR_RELEASE_NAME_TEMPLATE env var) instead of the default (which would be metrics-server-2.11.2 with the current config).

appVersion: 0.3.6
description: Metrics Server is a cluster-wide aggregator of resource usage data.
name: metrics-server
version: 2.11.2

Choose a reason for hiding this comment

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

Please bump the version when moving to a new chart repo

Comment on lines +14 to +15
- name: kennethaasan
email: [email protected]

Choose a reason for hiding this comment

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

Has @kennethaasan also agreed to continue maintaining this chart in the new repo? Just checking.

uses: helm/[email protected]
with:
command: lint
config: ct.yaml

Choose a reason for hiding this comment

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

This PR should also include a ct.yaml chart-testing config file https://github.com/helm/chart-testing#configuration

@scottrigby
Copy link

I'm unsure how we can accept this without the CLA status being resolved. I'm generally happy with this change, but I think we will need each person to sign the CLA to move forward with this if we want to preserve history.

Except for a few recent commits (and very early ones), all of which can be fixed – the commits contain DCO signoff. I believe CNCF projects require either DCO or CLA. Could that work here @brancz?

@bryankaraffa
Copy link

@olemarkus -- if I can help contribute and get some of those suggestions implemented, please let me know // grant push access.. Thanks for taking the initiative!

@stevehipwell
Copy link
Contributor

Is there any progress on this?

@llamahunter
Copy link

What needs to happen to get this merged? Rebase against master?

@pierluigilenoci
Copy link
Contributor

What needs to happen to get this merged? Rebase against master?

Probably it is enough to resolve the README.md conflict.

@pierluigilenoci
Copy link
Contributor

@olemarkus could you please take a look?

@stevehipwell
Copy link
Contributor

@pierluigilenoci I've added an inline comment for @olemarkus to look at the release.yaml config that was introduced by kubernetes-sigs/descheduler#436. This config has the latest GitHub actions (tested) and the ability to use a custom tag for the Helm chart releases (untested).

@olemarkus
Copy link
Author

Hi. I am afraid I am a bit too busy now before the holidays to have a deeper look into this.
Where are we with the legal questions above? I also believe that a second maintainer is still missing?

@llamahunter
Copy link

what's the status of this work? does it seem likely to be finished by the end of the year?

@scottrigby
Copy link

scottrigby commented Dec 22, 2020

Where are we with the legal questions above? I also believe that a second maintainer is still missing?

@olemarkus Since my comment #606 (comment) we have addressed this in another k8s chart relocation (kubernetes/kube-state-metrics#1237) and could follow up with CNCF in the same way for this repo. CC @brancz @s-urbaniak @serathius

@pierluigilenoci
Copy link
Contributor

I wouldn't bother but I really need an official chart for the metrics server. Is there any news?

@pierluigilenoci
Copy link
Contributor

@olemarkus could you please take a look?

@olemarkus
Copy link
Author

As mentioned in #572 I am co-maintaining a kOps metrics-server addon instead that will do much of the same as the helm chart + some tighter TLS integrations with the cluster provisioner. At the moment, I don't have the time to contribute to the helm chart in addition.

I suggest discussion the completion of this in the issue instead of this PR. And close this as it probably never will be merged.

/close

@olemarkus olemarkus closed this Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Official Helm Chart