chore(localstack): add extraLabels option and align documentation #128
Conversation
There was a problem hiding this comment.
Awesome! Thanks, @levivannoort for the contribution! It is really looking great! I only had one comment we need to address before we can merge this! 🚀
| | `extraDeploy` | Extra objects to deploy (value evaluated as a template) | `{}` | | ||
| | `extraAnnotations` | Add additional annotations to every resource (value evaluated as a template) | `{}` | |
There was a problem hiding this comment.
praise: Thanks for adding docs on already existing features!
There was a problem hiding this comment.
The whole values files could be made to adhere to a predefined structure in which the readme in it's current form is generated based on the default values file - it does seem like some of the values have these 'annotations' that are needed, e.g., https://github.com/bitnami/readme-generator-for-helm
There was a problem hiding this comment.
Sure, thanks for the tip, that's already on the bucket list 😄
There was a problem hiding this comment.
If I have some additional time I could create a pull request for it!
| {{- if .Values.extraLabels }} | ||
| {{ toYaml .Values.extraLabels }} | ||
| {{- end }} |
There was a problem hiding this comment.
suggestion: The evaluation of the labels does not yet include the tpm directive to evaluate the content of the value as a template itself (as mentioned in your docs addition). So we would either have to implement this similar to the localstack.annotations (see here) or remove the "(value evaluated as a template)" from the README.
There was a problem hiding this comment.
Aligned the behavior for extraLabels with extraAnnotations to have consistency across the template, thanks for pointing it out.
| @@ -40,6 +40,9 @@ helm.sh/chart: {{ include "localstack.chart" . }} | |||
| app.kubernetes.io/version: {{ .Chart.AppVersion | quote }} | |||
| {{- end }} | |||
| app.kubernetes.io/managed-by: {{ .Release.Service }} | |||
| {{- if .Values.extraLabels }} | |||
| {{ toYaml .Values.extraLabels }} | |||
There was a problem hiding this comment.
question: You mentioned in your PR description that we could also use commons.labels here. I do not have a strong opinion on this, so I'd leave this up to you. The upside is that you could create a cross-chart UX since for all charts that use commons.labels, setting global labels works like this. On the other hand we have not been consistently using the commons chart features yet, so calling it extraLabels is more consistent in the usage of this chart.
There was a problem hiding this comment.
I'd probably opt for going with the extraLabels to indeed have consistent usage in the chart for now, and later adopt the common chart with more of the template options if needed.
| | `extraDeploy` | Extra objects to deploy (value evaluated as a template) | `{}` | | ||
| | `extraAnnotations` | Add additional annotations to every resource (value evaluated as a template) | `{}` | |
Motivation
We're using
Localstackwhen spinning up temporary pull request environments. I would like to useKyvernoto act upon a time-to-live (ttl) label set on all of the resources with the temporary pull request environments. Although it had a anextraAnnotationsoption, I am missing anextraLabelsoption.Another option would be to use the
common.labelsof the chart dependency.Changes
Added a
extraLabelsvalue which is evaluated through the template.Testing
Add a key & value under the
extraLabels, an example can be see below with theKyvernotime-to-live (ttl) label.