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

enhancement(helm platform): Kubernetes recommanded labels and additional labels #7687

Merged
merged 10 commits into from Jun 7, 2021

Conversation

nlamirault
Copy link
Contributor

Signed-off-by: Nicolas Lamirault nicolas.lamirault@gmail.com

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>
Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>
@nlamirault nlamirault changed the title Kubernetes recommanded labels and additional labels enhancement(helm platform): Kubernetes recommanded labels and additional labels May 31, 2021
Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>
@nlamirault nlamirault marked this pull request as ready for review May 31, 2021 16:50
@nlamirault nlamirault requested review from a team and jszwedko and removed request for a team May 31, 2021 16:50
Copy link
Contributor

@spencergilbert spencergilbert left a comment

Choose a reason for hiding this comment

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

Left a couple of thoughts

distribution/helm/vector-shared/templates/_labels.tpl Outdated Show resolved Hide resolved
distribution/helm/vector-shared/templates/_labels.tpl Outdated Show resolved Hide resolved
Copy link
Contributor

@spencergilbert spencergilbert left a comment

Choose a reason for hiding this comment

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

Thanks for adding this @nlamirault - I've left a couple more notes and requests

distribution/helm/vector-aggregator/values.yaml Outdated Show resolved Hide resolved
distribution/helm/vector-agent/values.yaml Outdated Show resolved Hide resolved
distribution/helm/vector-shared/templates/_labels.tpl Outdated Show resolved Hide resolved
distribution/helm/vector-shared/templates/_labels.tpl Outdated Show resolved Hide resolved
Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>
Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>
Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>
@spencergilbert
Copy link
Contributor

Sorry @nlamirault - I realize I wasn't super clear. For the global.commonLabels we should put it here, like the existing commonEnvKV.

This is an example of us determining which value (global vs "local") but I can see a use for defining at both levels at the same time, so I think we can just do an if ...toYaml global.commonLabels end followed by the same but for "local."

Does that seem reasonable to you?

@nlamirault
Copy link
Contributor Author

Sorry @nlamirault - I realize I wasn't super clear. For the global.commonLabels we should put it here, like the existing commonEnvKV.

This is an example of us determining which value (global vs "local") but I can see a use for defining at both levels at the same time, so I think we can just do an if ...toYaml global.commonLabels end followed by the same but for "local."

Does that seem reasonable to you?

like ?

{{- if .Values.global }}
{{- if .Values.global.vector }}
{{- if .Values.global.vector.commonLabels }}
{{ toYaml .Values.global.vector.commonLabels }}
{{- end }}
{{- end }}
{{- end }}

{{- if .Values.commonLabels }}
{{ toYaml .Values.commonLabels }}
{{- end }}

and :

global:
  vector:
    # Additional labels to add to all resources
    commonLabels: #{}
      app: vector
commonLabels: #{}
  app.kubernetes.io/component: agent
commonLabels: #{}
  app.kubernetes.io/component: aggregator

@spencergilbert
Copy link
Contributor

Yep - that looks like it should work 👍

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>
Copy link
Contributor

@spencergilbert spencergilbert left a comment

Choose a reason for hiding this comment

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

Thanks - @jszwedko do you have any more thoughts here? We will be adding some more labels by default, that doesn't really bother me but wasn't sure if you had an opinion

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>
Copy link
Contributor

@spencergilbert spencergilbert left a comment

Choose a reason for hiding this comment

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

Thanks again for the updates, left some suggestions to keep the existing behavior by default.

distribution/helm/vector/values.yaml Outdated Show resolved Hide resolved
distribution/helm/vector/values.yaml Outdated Show resolved Hide resolved
distribution/helm/vector-aggregator/values.yaml Outdated Show resolved Hide resolved
distribution/helm/vector-agent/values.yaml Outdated Show resolved Hide resolved
Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>
Copy link
Contributor

@spencergilbert spencergilbert left a comment

Choose a reason for hiding this comment

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

Thanks so much @nlamirault, really appreciate the PR 😃 mind re-generating the resource files and I'll merge once the check is green

Signed-off-by: Nicolas Lamirault <nicolas.lamirault@gmail.com>
@spencergilbert spencergilbert merged commit ba230d4 into vectordotdev:master Jun 7, 2021
@spencergilbert
Copy link
Contributor

Thanks!

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

2 participants