Update metrics-server chart to show in kubectl cluster-info #12608

goshlanguage · 2019-03-28T03:10:35Z

What this PR does / why we need it:

This PR adds metrics-server to kubectl cluster-info. This seems appropriate, as Heapster used to report in cluster-info.

Before:

λ ~/ kubectl cluster-info
Kubernetes master is running at https://192.168.0.1:6443
KubeDNS is running at https://192.168.0.1:6443/api/v1/namespaces/kube-system/services/kube-dns:dns/proxy

After

λ ~/ kubectl cluster-info
Kubernetes master is running at https://192.168.0.1:6443
KubeDNS is running at https://192.168.0.1:6443/api/v1/namespaces/kube-system/services/kube-dns:dns/proxy
Metrics-server is running at https://192.168.0.1:6443/api/v1/namespaces/kube-system/services/https:metrics-server:/proxy

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

fixes #

Special notes for your reviewer:

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

DCO signed
Chart Version bumped
Variables are documented in the README.md

k8s-ci-robot · 2019-03-28T03:10:49Z

Hi @ryanhartje. Thanks for your PR.

I'm waiting for a helm 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.

goshlanguage · 2019-04-09T14:28:51Z

@olemarkus @kennethaasan anything I can do to move this along before it gets stale?

olemarkus

Sorry for missing this one. Just bump the major version and this should be good to go

olemarkus · 2019-04-09T16:06:15Z

stable/metrics-server/Chart.yaml

@@ -2,7 +2,7 @@ apiVersion: v1
 appVersion: 0.3.1
 description: Metrics Server is a cluster-wide aggregator of resource usage data.
 name: metrics-server
-version: 2.5.1
+version: 2.5.2


Can you bump minor version?

Actually, why would we update the minor version? Did you mean the patch version? The current version of the chart is already bumped appropriately. If you disagree, can you please elaborate?

@olemarkus I'm not seeing any merge conflicts or otherwise and am confused by the above, please advise.

Any new feature should be a minor increase, as @olemarkus suggested. You added a backwards compatible new option serviceLabels that didn't exist before and according to semver that warrants a minor bump.

olemarkus · 2019-04-09T16:07:01Z

/ok-to-test

steven-sheehy · 2019-04-13T03:30:54Z

stable/metrics-server/values.yaml

-# priorityClassName: system-node-critical
+serviceLabels: {}
+# Enable the next two labels to show metrics-server when running kubectl cluster-info
+#  kubernetes.io/cluster-service: "true"


Any downside to enabling these by default?

I chose not to, as I don't want to prescribe my solution.

Not adding this label by default is probably reasonable as I believe it has no meaning outside of the kube-system namespace. Before I was aware of this pending change, I opened a PR to add the label to the official example deployment: kubernetes-sigs/metrics-server#252

jhoblitt · 2019-04-18T18:30:13Z

stable/metrics-server/templates/metric-server-service.yaml

@@ -7,6 +7,9 @@ metadata:
    chart: {{ template "metrics-server.chart" . }}
    release: {{ .Release.Name }}
    heritage: {{ .Release.Service }}
+{{- with .Values.serviceLabels }}


As .Values.service.annotations has been merged, this should probably be changed to .Values.service.labels for consistency.

jhoblitt · 2019-04-18T18:32:24Z

It looks like this PR needs to be rebased, the version string updated, and the label key names changed to be consistent with the current state of the chart.

stable/metrics-server/values.yaml

stable/metrics-server/README.md

olemarkus · 2019-05-15T09:13:03Z

/assign

tariq1890 · 2019-05-16T16:47:37Z

@ryanhartje Can you resolve the merge conflicts and squash the commit history?

stable/metrics-server/README.md

goshlanguage · 2019-05-17T14:44:38Z

Thanks @tariq1890, @jhoblitt, @steven-sheehy, @olemarkus for your feedback.

tariq1890 · 2019-05-17T21:42:21Z

stable/metrics-server/README.md

@@ -29,6 +29,7 @@ Parameter | Description | Default
 `priorityClassName` | Pod priority class | `""`
 `readinessProbe` | Container readiness probe | See values.yaml
 `service.annotations` | Annotations to add to the service | `{}`
+`service.labels` | labels to be added to the metrics-server service | `{}`


Suggested change

`service.labels` | labels to be added to the metrics-server service | `{}`

`service.labels` | Labels to be added to the metrics-server service | `{}`

I recommend applying this manually and force pushing this :).

why edit this only, as opposed to all of these?

I think you misunderstood my review comment. The uppercasing of the first letter was only meant for the Description, not the value field.

Yep, I sure did.

tariq1890 · 2019-05-17T21:43:48Z

stable/metrics-server/templates/metric-server-service.yaml

@@ -7,6 +7,9 @@ metadata:
    chart: {{ template "metrics-server.chart" . }}
    release: {{ .Release.Name }}
    heritage: {{ .Release.Service }}
+{{- with .Values.service.labels }}


Can we format this properly here?

I've updated them all, I think this is what we want here.

tariq1890 · 2019-05-17T21:44:17Z

Last round of review comments. Your patience is greatly appreciated @ryanhartje!

goshlanguage · 2019-05-18T22:12:33Z

@tariq1890 no worries, thanks for reviewing

tariq1890 · 2019-05-20T06:20:40Z

/test pull-charts-e2e

tariq1890 · 2019-05-20T06:23:45Z

stable/metrics-server/README.md

-`service.annotations` | Annotations to add to the service | `{}`
-`service.port` | Service port to expose | `443`
-`service.type` | Type of service to create | `ClusterIP`
+`service.Annotations` | Annotations to add to the service | `{}`


Can we change all of these service var names back to camelCase please?

Signed-off-by: ryanhartje <ryan.hartje@microsoft.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> fixed formatting Signed-off-by: ryanhartje <ryan.hartje@microsoft.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> forgot to sign off. Signed-off-by: ryanhartje <ryan.hartje@microsoft.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> bump version and append values table in README.md Signed-off-by: ryanhartje <ryan.hartje@microsoft.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> Update stable/metrics-server/README.md Co-Authored-By: Tariq Ibrahim <tariq181290@gmail.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> Update stable/metrics-server/values.yaml Co-Authored-By: Tariq Ibrahim <tariq181290@gmail.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> implementing feedback for metrics-server chart. Bumped chart version, updated metric-server-service template with server.labels value Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> adding new line at EOF for values in metrics-server chart to pass lint Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> can't win with these autoformatted spaces at the end of my values file... Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> [stable/metrics-server] Fix readme typo Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> Update metrics-server chart to show up in kubectl cluster-info Signed-off-by: ryanhartje <ryan.hartje@microsoft.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> fixed formatting Signed-off-by: ryanhartje <ryan.hartje@microsoft.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> forgot to sign off. Signed-off-by: ryanhartje <ryan.hartje@microsoft.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> bump version and append values table in README.md Signed-off-by: ryanhartje <ryan.hartje@microsoft.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> Update stable/metrics-server/README.md Co-Authored-By: Tariq Ibrahim <tariq181290@gmail.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> Update stable/metrics-server/values.yaml Co-Authored-By: Tariq Ibrahim <tariq181290@gmail.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> implementing feedback for metrics-server chart. Bumped chart version, updated metric-server-service template with server.labels value Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> adding new line at EOF for values in metrics-server chart to pass lint Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> can't win with these autoformatted spaces at the end of my values file... Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> [stable/metrics-server] Fix readme typo Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> [stable/metrics-server] updating service variable formatting to be consistent Signed-off-by: Ryan Hartje <ryan@ryanhartje.com> [stable/metrics-server] oof, mismatch for vars between values.yaml and the templates Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> [stable/metrics-server] reverting service values to camelCase as they should be Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> [stable/metrics-server] updating template format Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com>

tariq1890 · 2019-05-20T15:05:03Z

/lgtm

k8s-ci-robot · 2019-05-20T15:05:04Z

@tariq1890: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

/lgtm

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.

tariq1890 · 2019-05-20T15:06:28Z

/approve

olemarkus · 2019-05-20T17:34:55Z

/lgtm

k8s-ci-robot · 2019-05-20T17:34:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: olemarkus, ryanhartje, tariq1890

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:

~~stable/metrics-server/OWNERS~~ [olemarkus]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…12608) Signed-off-by: ryanhartje <ryan.hartje@microsoft.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> fixed formatting Signed-off-by: ryanhartje <ryan.hartje@microsoft.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> forgot to sign off. Signed-off-by: ryanhartje <ryan.hartje@microsoft.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> bump version and append values table in README.md Signed-off-by: ryanhartje <ryan.hartje@microsoft.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> Update stable/metrics-server/README.md Co-Authored-By: Tariq Ibrahim <tariq181290@gmail.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> Update stable/metrics-server/values.yaml Co-Authored-By: Tariq Ibrahim <tariq181290@gmail.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> implementing feedback for metrics-server chart. Bumped chart version, updated metric-server-service template with server.labels value Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> adding new line at EOF for values in metrics-server chart to pass lint Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> can't win with these autoformatted spaces at the end of my values file... Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> [stable/metrics-server] Fix readme typo Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> Update metrics-server chart to show up in kubectl cluster-info Signed-off-by: ryanhartje <ryan.hartje@microsoft.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> fixed formatting Signed-off-by: ryanhartje <ryan.hartje@microsoft.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> forgot to sign off. Signed-off-by: ryanhartje <ryan.hartje@microsoft.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> bump version and append values table in README.md Signed-off-by: ryanhartje <ryan.hartje@microsoft.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> Update stable/metrics-server/README.md Co-Authored-By: Tariq Ibrahim <tariq181290@gmail.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> Update stable/metrics-server/values.yaml Co-Authored-By: Tariq Ibrahim <tariq181290@gmail.com> Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> implementing feedback for metrics-server chart. Bumped chart version, updated metric-server-service template with server.labels value Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> adding new line at EOF for values in metrics-server chart to pass lint Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> can't win with these autoformatted spaces at the end of my values file... Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> [stable/metrics-server] Fix readme typo Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> [stable/metrics-server] updating service variable formatting to be consistent Signed-off-by: Ryan Hartje <ryan@ryanhartje.com> [stable/metrics-server] oof, mismatch for vars between values.yaml and the templates Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> [stable/metrics-server] reverting service values to camelCase as they should be Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com> [stable/metrics-server] updating template format Signed-off-by: Ryan Hartje <ryan.hartje@microsoft.com>

helm-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Mar 28, 2019

k8s-ci-robot requested review from kennethaasan and olemarkus March 28, 2019 03:10

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 28, 2019

goshlanguage force-pushed the cluster-info-labels branch from 95f5654 to 1eec534 Compare March 28, 2019 03:18

helm-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 28, 2019

olemarkus suggested changes Apr 9, 2019

View reviewed changes

k8s-ci-robot added ok-to-test and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 9, 2019

steven-sheehy reviewed Apr 13, 2019

View reviewed changes

jhoblitt reviewed Apr 18, 2019

View reviewed changes

tariq1890 reviewed May 14, 2019

View reviewed changes

stable/metrics-server/values.yaml Outdated Show resolved Hide resolved

tariq1890 reviewed May 14, 2019

View reviewed changes

stable/metrics-server/README.md Outdated Show resolved Hide resolved

k8s-ci-robot assigned olemarkus May 15, 2019

helm-bot removed the Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). label May 16, 2019

helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 16, 2019

goshlanguage force-pushed the cluster-info-labels branch from 87e45a4 to 2347ead Compare May 16, 2019 17:36

helm-bot added Contribution Allowed If the contributor has signed the DCO or the CNCF CLA (prior to the move to a DCO). size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 16, 2019

tariq1890 reviewed May 16, 2019

View reviewed changes

stable/metrics-server/README.md Outdated Show resolved Hide resolved

helm-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 17, 2019

tariq1890 reviewed May 17, 2019

View reviewed changes

goshlanguage force-pushed the cluster-info-labels branch from 3230eae to 3e45224 Compare May 18, 2019 22:08

tariq1890 reviewed May 20, 2019

View reviewed changes

goshlanguage force-pushed the cluster-info-labels branch 2 times, most recently from a538239 to 9fcb796 Compare May 20, 2019 14:28

helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 20, 2019

goshlanguage force-pushed the cluster-info-labels branch from 9fcb796 to 67575da Compare May 20, 2019 14:38

helm-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 20, 2019

goshlanguage force-pushed the cluster-info-labels branch from 67575da to 7834765 Compare May 20, 2019 14:54

k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label May 20, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 20, 2019

k8s-ci-robot merged commit efa091f into helm:master May 20, 2019

goshlanguage deleted the cluster-info-labels branch May 20, 2019 20:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update metrics-server chart to show in kubectl cluster-info #12608

Update metrics-server chart to show in kubectl cluster-info #12608

goshlanguage commented Mar 28, 2019 •

edited

Loading

k8s-ci-robot commented Mar 28, 2019

goshlanguage commented Apr 9, 2019

olemarkus left a comment

olemarkus Apr 9, 2019

goshlanguage Apr 9, 2019

goshlanguage Apr 9, 2019

goshlanguage Apr 10, 2019

steven-sheehy Apr 13, 2019 •

edited

Loading

olemarkus commented Apr 9, 2019

steven-sheehy Apr 13, 2019

goshlanguage Apr 16, 2019

jhoblitt Apr 18, 2019

jhoblitt Apr 18, 2019

jhoblitt commented Apr 18, 2019

olemarkus commented May 15, 2019

tariq1890 commented May 16, 2019

goshlanguage commented May 17, 2019

tariq1890 May 17, 2019

tariq1890 May 17, 2019

goshlanguage May 18, 2019

tariq1890 May 20, 2019

goshlanguage May 20, 2019

tariq1890 May 17, 2019

goshlanguage May 18, 2019

tariq1890 commented May 17, 2019

goshlanguage commented May 18, 2019

tariq1890 commented May 20, 2019

tariq1890 May 20, 2019

tariq1890 commented May 20, 2019

k8s-ci-robot commented May 20, 2019

tariq1890 commented May 20, 2019

olemarkus commented May 20, 2019

k8s-ci-robot commented May 20, 2019

	`service.labels` \| labels to be added to the metrics-server service \| `{}`
	`service.labels` \| Labels to be added to the metrics-server service \| `{}`

Update metrics-server chart to show in kubectl cluster-info #12608

Update metrics-server chart to show in kubectl cluster-info #12608

Conversation

goshlanguage commented Mar 28, 2019 • edited Loading

What this PR does / why we need it:

Which issue this PR fixes

Special notes for your reviewer:

Checklist

k8s-ci-robot commented Mar 28, 2019

goshlanguage commented Apr 9, 2019

olemarkus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steven-sheehy Apr 13, 2019 • edited Loading

Choose a reason for hiding this comment

olemarkus commented Apr 9, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhoblitt commented Apr 18, 2019

olemarkus commented May 15, 2019

tariq1890 commented May 16, 2019

goshlanguage commented May 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tariq1890 commented May 17, 2019

goshlanguage commented May 18, 2019

tariq1890 commented May 20, 2019

Choose a reason for hiding this comment

tariq1890 commented May 20, 2019

k8s-ci-robot commented May 20, 2019

tariq1890 commented May 20, 2019

olemarkus commented May 20, 2019

k8s-ci-robot commented May 20, 2019

goshlanguage commented Mar 28, 2019 •

edited

Loading

steven-sheehy Apr 13, 2019 •

edited

Loading