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

Recover fallback on server configuration when resourceLabels are not specified #580

Conversation

mark-hofmeijer
Copy link
Contributor

What was changed

This PR recovers the possibility to define resource labels on server level (e.g. server.podLabels) rather than only within the scope of the server services (frontend, history, matching, worker).

Why?

With the introduction of resourceLabels templatization (#539), the option to specify podLabels on server level has been discontinued. Presumably this was not done consciously.

In the old situation it worked as follows:
{{- with (default $.Values.server.podLabels $serviceValues.podLabels) }}
(source: https://github.com/temporalio/helm-charts/pull/539/files#diff-b8142966d328045abc62158c9a0c7b8492a97eb135757aea4a6941d4a80edb9e)

But in the new situation, in case the component equals server only the resource labels (e.g. server.frontend.podLabels) within the scope are used. If not specified, any values in server.podLabels are ignored.

Checklist

  1. How was this tested:
    helm install -f values.yaml --set server.frontend.podLabels.resourceLabel1="resourceTest1" debug . --dry-run --debug
  • This should add a label resourceLabel1 to the temporal-frontend deployment.
  • This is the case in both the current and the new situation.

helm install -f values.yaml --set server.podLabels.serverLabel1="serverTest1" --set server.frontend.podLabels.resourceLabel1="resourceTest1" debug . --dry-run --debug

  • This should add a label resourceLabel1 to the temporal-frontend deployment.
  • This is the case in both the current and the new situation.

helm install -f values.yaml --set server.podLabels.serverLabel1="serverTest1" debug . --dry-run --debug

  • This should add a label serverLabel1 to the temporal-frontend deployment (and other server services).
  • In the current situation no labels are added to any of the server services.
  • In the new situation label serverLabel1 is added to all server services.
  1. Any docs updates needed?
    No.

@robholland
Copy link
Contributor

Thanks for the PR, it does indeed fix the issue you mentioned, but I'd like to take a slightly different approach and merge the two. I've also added some tests, please see what you think: #584

@mark-hofmeijer
Copy link
Contributor Author

@robholland Totally agree that merging is a better solution. I have doubted this myself, but did not adjust because it was not implemented that way initially and I wanted to create as little impact as possible.

So as far as I'm concerned, it's fine to continue with #584

@robholland
Copy link
Contributor

Closed in favour of #584

@robholland robholland closed this Oct 9, 2024
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.

2 participants