Update Config Templating#786
Conversation
Temporal core allows for the registration and configuration of custom data stores.
However, there is a disparity in how the helm chart translates persistence
configuration between helm values and the resulting configmap.
As an example, the temporal config for a custom driver for the default store may look
as so:
```
persistence:
defaultStore: my-custom-store
visibilityStore: es-visibility
numHistoryShards: 4
datastores:
my-custom-store:
customDatastore:
name: "my-custom"
options:
hosts: "mycustom.example.com"
keyspace: "temporal"
```
Translating to helm values would look like
```
server:
config:
persistence:
defaultStore: my-custom-store
additionalStores:
my-custom-store:
customDatastore:
name: "my-custom"
options:
hosts: "mycustom.example.com"
keyspace: "temporal"
```
This gets us most of the way there, but there are a few problems:
1. The charts always assume defaultStore = "default" and thus have opinions
about subordinate structures such as default.driver. This results in objects
such as Secrets and Jobs being incorrectly emitted for default.driver = "cassandra" even
though we are trying to specify an alternate store. This logic should probably
be $defaultStore.driver and $visibilityStore.driver instead.
2. The additionalStores configuration does not lend itself to first-class
representation in conditional statements, even though it can be used to
emit the resulting configmap correctly.
One could argue that we should clean up the helm representation to more closely align
with the temporal-server yaml, but this is a bigger task (and a breaking change). Instead,
this patch ties into the existing model of using $store.driver = {cassandra, sql, elasticsearch}
and adds a 4th first-class type "custom."
The user may configure this as so:
```
server:
config:
persistence:
default:
driver: custom
visibility:
driver: custom
```
And in doing so, we instruct the various templates to elide the objects (Jobs, Secrets, etc.)
related to that store, leaving it to the user to supply the rest of the required configuration
using a combination of additionalStores, additionalEnv, additionalVolumeMounts, etc., to inject
configuration and secrets. They would also, of course, be responsible for any DDL setup or
migration.
Signed-off-by: Greg Haskins <greg@manetu.com>
This change was made by an automated process to ensure all GitHub Actions workflows have explicitly defined permissions as per best practices.
* Update the contributing docs for clarity. Help to guide users on what kind of changes we will accept. --------- Co-authored-by: Tom Wheeler <tom@temporal.io>
what: fixed a very minor typo in README.md
bergundy
left a comment
bergundy
left a comment
There was a problem hiding this comment.
I think it would have been better to support the .Env syntax to avoid an unnecessary breaking change.
Make sure to avoid merging this until the server images are updated.
|
There are multiple ways in which the new template syntax is different from |
After some internal conversation we are going with |
|
I'd like to introduce this into the alpha 1.0 builds rather than the 0.x range so all breaking changes occur there. Please switch the PR to target the |
| maxJoinDuration: 30s | ||
| broadcastAddress: {{ `{{ default .Env.POD_IP "0.0.0.0" }}` }} | ||
| # TODO: validate broadcast address can work with 0.0.0.0 | ||
| broadcastAddress: {{ `{{ env "POD_IP" | default "" }}` }} |
There was a problem hiding this comment.
Presumably we can remove the default here?
robholland
left a comment
robholland
left a comment
There was a problem hiding this comment.
Please switch to target v1 branch.
|
|
|
|
|
Semgrep found 1 No explicit |
|
moving to #794 |
11 participants
What was changed
add required comment to enable templating and updated the
envsyntax to usesprig. Existing syntax if fordockerize.Why?
The config yaml includes values than are intended to be read from the environment by
sprig. The server now requires that# enable-templatecomment to allow this.How was this tested:
running the helm chart with the new docker images. Everything starts successfully after this comment is added.
Breaking change: existing server versions will not be able to parse this config-map