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.

Sign up for GitHub

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

chore(localstack): add extraLabels option and align documentation #128

Merged
merged 2 commits into from
Nov 27, 2024
Merged

chore(localstack): add extraLabels option and align documentation #128

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
024d17c
chore(localstack): add extraLabels option and align documentation
levivannoort Nov 27, 2024
84a3b04
fix(localstack): align template extraLabels with extraAnnotations
levivannoort Nov 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions charts/localstack/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,12 @@ The following table lists the configurable parameters of the Localstack chart an
| `image.tag` | Localstack image tag | `latest` |
| `image.pullPolicy` | Localstack image pull policy | `IfNotPresent` |
| `image.pullSecrets` | Specify docker-registry secret names as an array | `[]` |
| `podLabels` | Additional pod labels for Localstack secondary pods | `{}` |
| `podLabels` | Additional pod labels for Localstack secondary pods | `{}` |
| `podAnnotations` | Additional pod annotations for Localstack secondary pods | `{}` |
| `podSecurityContext` | Enable security context for Localstack pods | `{}` |
| `extraDeploy` | Extra objects to deploy (value evaluated as a template) | `{}` |
| `extraAnnotations` | Add additional annotations to every resource (value evaluated as a template) | `{}` |
Comment on lines +98 to +99
Copy link
Member

Choose a reason for hiding this comment

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

praise: ‏Thanks for adding docs on already existing features!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@alexrashed alexrashed Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Sure, thanks for the tip, that's already on the bucket list 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I have some additional time I could create a pull request for it!

Copy link
Member

Choose a reason for hiding this comment

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

That would be great :)

| `extraLabels` | Add additional labels to every resource (value evaluated as a template) | `{}` |
| `securityContext` | Localstack container securityContext | `{}` |

### Localstack parameters
Expand All @@ -103,11 +106,11 @@ The following table lists the configurable parameters of the Localstack chart an
|------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|---------------------------------------------------------|
| `debug` | Specify if debug logs should be enabled | `false` |
| `kinesisErrorProbability` | Specify to randomly inject ProvisionedThroughputExceededException errors into Kinesis API responses | `nil` (Localstack Default) |
| `startServices` | Comma-separated list of AWS CLI service names which should be loaded right when starting LocalStack. If not set, each service is loaded and started on the first request for that service. | `nil` (Localstack Default) |
| `startServices` | Comma-separated list of AWS CLI service names which should be loaded right when starting LocalStack. If not set, each service is loaded and started on the first request for that service. | `nil` (Localstack Default) |
| `lambdaExecutor` | Specify Method to use for executing Lambda functions (partially supported) | `docker` |
| `extraEnvVars` | Extra environment variables to be set on Localstack primary containers | `nil` (Localstack Default) |
| `enableStartupScripts` | Mount `/etc/localstack/init/ready.d` to run startup scripts with `{{ template "localstack.fullname" . }}-init-scripts-config` configMap | `false` |
| `startupScriptContent` | Startup script content when `enableStartupScripts` is `true`. Note: You will need to add a shebang as your first line such as `!#/bin/sh` in order to ensure the startup script is not malformed. | `nil` (Localstack Default) |
| `startupScriptContent` | Startup script content when `enableStartupScripts` is `true`. Note: You will need to add a shebang as your first line such as `!#/bin/sh` in order to ensure the startup script is not malformed. | `nil` (Localstack Default) |

### Deployment parameters

Expand Down
3 changes: 3 additions & 0 deletions charts/localstack/templates/_helpers.tpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 }}
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

{{- end }}
Copy link
Member

Choose a reason for hiding this comment

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

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.‏

Copy link
Contributor Author

@levivannoort levivannoort Nov 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Aligned the behavior for extraLabels with extraAnnotations to have consistency across the template, thanks for pointing it out.

{{- end }}

{{/*
Expand Down
5 changes: 4 additions & 1 deletion charts/localstack/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,12 @@ fullnameOverride: ""
##
extraDeploy: []

## Add additional annotations to every resource
## @param extraAnnotations Add additional annotations to every resource (value evaluated as a template)
extraAnnotations: {}

## @param extraLabels Add additional labels to every resource (value evaluated as a template)
extraLabels: {}

serviceAccount:
# Specifies whether a service account should be created
create: true
Expand Down
Loading