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: Populate SQL_CONNECT_ATTRIBUTES in admintools and schema job #567

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

adigiorgi-clickup
Copy link

What was changed

When server.persistence.config.*.sql.connectAttributes is set, also populate the SQL_CONNECT_ATTRIBUTES env var in the temporal-schema job. According to https://github.com/temporalio/temporal/blob/main/tools/sql/handler.go#L157, the SQL_CONNECT_ATTRIBUTE env var must be in this format: key1=value1&key2=value.

Why?

If users want to use a different schema than public, they need to set the connectAttribute value to something like search_path: temporal. With this PR, the temporal-schema job will know about it and create tables in the right schema.

Checklist

  1. Closes [Feature Request] Ability to specify SQL_CONNECT_ATTRIBUTES for temporal-schema job #561

  2. How was this tested:

    1. helm lint charts/temporal
    2. Add connectAttributes to charts/temporal/values/values.postgresql.yaml:
    connectAttributes:
      search_path: temporal
      foo_bar: baz
    1. helm template my-release charts/temporal -f charts/temporal/values/values.postgresql.yaml
    2. Check the resulting yaml contains the SQL_CONNECT_ATTRIBUTES env var:
      - name: SQL_CONNECT_ATTRIBUTES
        value: "foo_bar=baz&search_path=temporal"
    3. Run step iii against main and against my branch, checking the resulting yamls are the same.

@adigiorgi-clickup adigiorgi-clickup requested a review from a team as a code owner September 27, 2024 22:21
@CLAassistant
Copy link

CLAassistant commented Sep 27, 2024

CLA assistant check
All committers have signed the CLA.

@adigiorgi-clickup
Copy link
Author

@robholland, sorry for the ping! Anything I can do to help push this forward?

@robholland robholland self-assigned this Nov 5, 2024
@robholland
Copy link
Contributor

Please merge https://github.com/temporalio/helm-charts/tree/rh-sql-connect-attributes which contains a failing test for this, and then adjust as required.

@robholland
Copy link
Contributor

You can run with go test -timeout 30s -run ^TestAdminToolsSqlConnectAttributes$ github.com/temporalio/helm-charts from the charts/temporal/tests directoy.

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.

[Feature Request] Ability to specify SQL_CONNECT_ATTRIBUTES for temporal-schema job
3 participants