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

Add support for deployment labels #581

Merged

Conversation

mark-hofmeijer
Copy link
Contributor

What was changed

This PR adds support to define deployment labels for the server, admintools and web components.

NOTE 1: this includes the option to define labels on server level (server.deploymentLabels) instead of within scope of the server services (frontend, history, matching, worker) only. However to make this work, PR #580 first needs to be merged.

NOTE 2: this PR partly replaces #537, however this PR needs revision because of the resourceLabels templatization in PR #539.

Why?

Currently there is no option to define deployment labels.

Checklist

  1. How was this tested:
    helm install -f values.yaml --set server.deploymentLabels.serverLabel1="serverDeploymentLabel" --set admintools.deploymentLabels.serverLabel1="adminDeploymentLabel" --set web.deploymentLabels.serverLabel1="webDeploymentLabel" debug . --dry-run --debug
  • In the current situation, this adds no deployment labels on any of the components.
  • In the new situation, a deployment label serverLabel1 is added to all server services and admintools and web components.

helm install -f values.yaml --set server.deploymentLabels.serverLabel1="serverDeploymentLabel" --set server.frontend.deploymentLabels.resourceLabel1="resourceDeploymentLabel" debug . --dry-run --debug

  • In the current situation, this adds no deployment labels on the server frontend service.
  • In the new situation, a deployment label resourceLabel1 is added to the server frontend service. The deployment labels on server level are ignored correctly.
  • In the new situation (with PR Recover fallback on server configuration when resourceLabels are not specified #580 merged), a deployment label serverLabel1 is added to the other server services (history, matching, worker).
  1. Any docs updates needed?
    No.

@mark-hofmeijer mark-hofmeijer requested a review from a team as a code owner October 3, 2024 07:47
@robholland
Copy link
Contributor

If you are comfortable enough in Go, please could you add some tests for this to charts/temporal/tests?

@mark-hofmeijer
Copy link
Contributor Author

@robholland I'm fairly early in my Go journey, but based on the tests you recently added I think I've made it.

go test . '-run=^TestTemplateServerDeploymentLabels$'
ok github.com/temporalio/helm-charts 4.100s

Happy to hear your feedback!

@mark-hofmeijer
Copy link
Contributor Author

Added the "inline" refactor on the labels inline method and also added a test for the additional labels, as discussed in #582

@robholland
Copy link
Contributor

Please merge master to get the CI fix.

@robholland robholland merged commit 93165b8 into temporalio:main Oct 14, 2024
3 checks passed
asproul pushed a commit to asproul/helm-charts that referenced this pull request Dec 16, 2024
* feat: add support for defining deployment labels

---------

Co-authored-by: Rob Holland <[email protected]>
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