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

fix: add datadog service label value to chart #42

Merged
merged 1 commit into from
May 10, 2024

Conversation

davidmdm
Copy link
Contributor

No description provided.

@davidmdm davidmdm requested a review from alexstojda May 10, 2024 19:31
image:
tag: 0.1.2

datadogServiceLabel: joy-generator-chart-test
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a bit opinionated on having datadog.

I would go with a more generic approach of having an additionalLabels or, more specifically a podLabels field which is a map of labels & values. This is the approach most charts use.

Copy link

Choose a reason for hiding this comment

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

yeah, +1 for podLabels

@@ -34,6 +34,7 @@ Create chart name and version as used by the chart label.
Common labels
*/}}
{{- define "joy-generator.labels" -}}
tags.datadoghq.com/service: {{ .Values.datadogServiceLabel | default .Chart.Name }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
tags.datadoghq.com/service: {{ .Values.datadogServiceLabel | default .Chart.Name }}
tags.datadoghq.com/service: {{ .Values.datadogServiceLabel | default .Release.Name }}

What do you think about using the release name here instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds bueno!

@davidmdm davidmdm force-pushed the fix/add-datadog-service-label branch 3 times, most recently from 59d0ef9 to 14a0adc Compare May 10, 2024 19:47
@davidmdm davidmdm enabled auto-merge (rebase) May 10, 2024 19:53
Comment on lines 43 to 45
{{- if .Values.podLabels }}
{{ toYaml .Values.podLabels }}
{{- end -}}
Copy link
Contributor

Choose a reason for hiding this comment

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

Check your expected file, the labels here are not added to the pod, only the meta of resources. In other words the labels were added to the deployment, but not the pod template.

This block should go below this line

{{- include "joy-generator.selectorLabels" . | nindent 8 }}

@davidmdm davidmdm force-pushed the fix/add-datadog-service-label branch from 14a0adc to 8c43091 Compare May 10, 2024 20:00
Copy link
Contributor

@alexstojda alexstojda left a comment

Choose a reason for hiding this comment

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

Muy bien!

@davidmdm davidmdm disabled auto-merge May 10, 2024 20:15
@davidmdm davidmdm merged commit fe50929 into master May 10, 2024
5 checks passed
@davidmdm davidmdm deleted the fix/add-datadog-service-label branch May 10, 2024 20:15
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.

3 participants