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

[kong] add the ability to include adhoc pod labels #121

Merged
merged 4 commits into from
Apr 24, 2020

Conversation

aspring
Copy link
Contributor

@aspring aspring commented Apr 21, 2020

What this PR does / why we need it:

This PR adds the ability to include adhoc pod labels.

Checklist

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

  • PR is based off the current tip of the next branch and targets next, not master
  • New or modified sections of values.yaml are documented in the README.md
  • Title of the PR and commit headers start with chart name (e.g. [kong])

@CLAassistant
Copy link

CLAassistant commented Apr 21, 2020

CLA assistant check
All committers have signed the CLA.

@hbagdi
Copy link
Member

hbagdi commented Apr 22, 2020

I'm not against it but @aspring, can you elaborate on your use-case for this?

@aspring
Copy link
Contributor Author

aspring commented Apr 22, 2020

We run Kong with Istio and by adding labels to the pod spec they can be utilized by Istio for enhancing tracing as well as visualizations.

@hbagdi
Copy link
Member

hbagdi commented Apr 22, 2020

Interesting. I figured it would always be via annotations but I think label makes sense for this.

@hbagdi hbagdi requested a review from rainest April 22, 2020 21:12
@@ -36,6 +36,9 @@ spec:
labels:
{{- include "kong.metaLabels" . | nindent 8 }}
app.kubernetes.io/component: app
{{- if .Values.podLabels }}
{{ toYaml .Values.podLabels | nindent 8 }}
{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

Please update an existing test file to include a test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be all set. The following snippet from the test showing the app and environment labels added.

Node:         chart-testing-control-plane/172.17.0.2
Start Time:   Fri, 24 Apr 2020 03:22:06 +0000
Labels:       app=kong
              app.kubernetes.io/component=app
              app.kubernetes.io/instance=kong-5n1v8lzb7n
              app.kubernetes.io/managed-by=Tiller
              app.kubernetes.io/name=kong
              app.kubernetes.io/version=2
              environment=test

Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

This should probably make use of nested items in values.yaml that touch the various places that don't have their own annotation/label sections. Roughly:

metadata:
  deployment:
    labels:
    - foo
    - bar
  pod:
    labels:
    - foo
    - bar
    annotations:
      example.com/example: whatever

However given the existing podAnnotations we'd need to make a breaking change to support it. I don't think that's pressing though, and this is consistent with the existing format, so fine as-is.

@hbagdi hbagdi merged commit 5700233 into Kong:next Apr 24, 2020
@hbagdi
Copy link
Member

hbagdi commented Apr 24, 2020

@aspring Please claim your contributor swag: https://github.com/Kong/kong/blob/master/CONTRIBUTING.md#contributor-t-shirt. Thank you for your contribution.

rainest pushed a commit that referenced this pull request May 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants