Skip to content
This repository has been archived by the owner on Aug 25, 2021. It is now read-only.

Add ability to add additional labels to server pods #553

Merged
merged 3 commits into from
Jul 27, 2020

Conversation

peterklijn
Copy link
Contributor

I would like to add the ability to add additional labels to consul-server pods, the same way annotations can be added.

At the company I work for, we use specific Kubernetes pod labels to tell Fluentd which Elasticsearch index should be used to send logs to. With this change I can the additional label :)

@hashicorp-cla
Copy link

hashicorp-cla commented Jul 24, 2020

CLA assistant check
All committers have signed the CLA.

Copy link

@thisisnotashwin thisisnotashwin 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 for the PR @peterklijn . I just had a few comments on it. I tested this out and it worked great 😄

test/unit/server-statefulset.bats Outdated Show resolved Hide resolved
values.yaml Outdated Show resolved Hide resolved
Copy link

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

This is excellent. Thanks for your contribution @peterklijn !!

Copy link

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

Hey @peterklijn

I was going through some of our other templates and realized we can potentially use toYaml here to template in the values. This way instead of accepting a string, we could accept a yaml block.

extraLabels:
  label1: val
  label2: val

instead of

extraLabels: |
  "label1": "val"
  "label2": "val"

We have used toYaml in multiple places so it should hopefully be straightforward.
Sorry for requesting changes after approving it bud.

@peterklijn
Copy link
Contributor Author

No problem @ashwin-venkatesh :)

I've updated the extraLabels to use a yaml block.

Additionally I've updated the annotations to also support a yaml block (it currently supports both a yaml block and a string which is documented as deprecated). Let me know if that's ok :)

@lkysow
Copy link
Member

lkysow commented Jul 27, 2020

I think we should leave out the annotations as string deprecation from this PR and put it in a separate PR that does the deprecation across all places where annotations can be set so we don't have inconsistency across the chart between components.

Copy link

@thisisnotashwin thisisnotashwin left a comment

Choose a reason for hiding this comment

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

Thanks @peterklijn

@peterklijn
Copy link
Contributor Author

Thanks for the review @ashwin-venkatesh and @lkysow,

I've removed the annotation change and rebased with upstream

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants