concourse / concourse-chart

Helm chart to install Concourse

Apache License 2.0

143 stars 174 forks source link

feat: add web labels to deployment metadata #308

Closed jmccann closed 1 year ago

jmccann commented 2 years ago

Why do we need this PR?

Would like to provide labels not only to .spec.template.metadata.labels but also .metadata.labels.

Changes proposed in this pull request

Add values from .Values.web.labels to web-deployment's .metadata.labels.

Contributor Checklist

[x] Variables are documented in the README.md
[x] Which branch are you merging into? - master
- master is for changes related to the current release of the concourse/concourse:latest image and should be good to publish immediately
- dev is for changes related to the next release of Concourse (aka unpublished code on master in concourse/concourse)

Reviewer Checklist

This section is intended for the core maintainers only, to track review progress. Please do not fill out this section.

[ ] Code reviewed

[ ] Topgun tests run

[ ] Back-port if needed

[ ] Is the correct branch targeted? (master or dev)

xtremerui commented 2 years ago

Hi, thanks for the PR. Could you give some details about the difference between adding labels to spec and metadata?

jmccann commented 2 years ago

I believe having under .metadata affects the Deployment resource itself while having under .spec.template.metadata affects the pods/resources that are generated from the deployment.

This is a requirement coming from internal ... so if this can not be included then I'd have to fork this chart 😢

Also, seems examples provided from kubernetes itself has them under .metadata: https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/#web-application-with-a-database

For us we need it under both .metadata and .spec.template.metadata. Hope this helps!

xtremerui commented 2 years ago

@jmccann sounds like there are different meanings of adding labels at different location, though in your case they are with same value.

To not confuse the usage of those values, how about configuring them separately?

web.labels in values.yml should goes to metadata.labels in web-deployment.yml
web.podLabels in values.yml should goes to spec.template.metadata.labels in web-deployment.yml

This means a breaking change and the readme has to be updated accordingly.

jmccann commented 2 years ago

@xtremerui I think I made the changes you requested. Let me know if anything else is needed. Thanks!