Visibility config doesn't get rendered if visibility and default store types are different

[np-we360]

What are you really trying to do?

I'm trying to install Temporal with Cassandra as the default store and PostgreSQL as the visibility store.

Describe the bug

While rendering visibility store config, instead of checking for .visibility key, it was checking for .default. (The chart works when both your default and visibility store are same)

Responsible line of code: https://github.com/temporalio/helm-charts/blob/master/charts/temporal/templates/server-configmap.yaml#L60

Minimal Reproduction

server:
  config:
    persistence:
      default:
        cassandra:
         # cassandra config
        driver: cassandra
      defaultStore: default
      visibility:
        driver: sql
        sql:
           # sql config
          driver: postgres12

Environment/Versions

Not applicable.

[smktpd]

Isn't that because you didn't set up visibilityStore (under persistence)?

[np-we360]

Isn't that because you didn't set up visibilityStore (under persistence)?

That shouldn't be the case afaik. Please correct me if I'm wrong.

It defaults to "visibility" from my understanding of the template.

[smktpd]

You might be right. I don't understand some helm constructs, yet.
https://github.com/temporalio/helm-charts/blob/master/charts/temporal/templates/server-configmap.yaml#L60

    persistence:
      defaultStore: {{ $.Values.server.config.persistence.defaultStore }}
    {{- if or $.Values.elasticsearch.enabled $.Values.elasticsearch.external }}
      visibilityStore: es-visibility
    {{- else }}
      visibilityStore: visibility
    {{- end }}
    ...
      datastores:
        ...
        visibility:
          {{- if eq (include "temporal.persistence.driver" (list $ "default")) "sql" }}

includes temporal.persistence.driver, which is defined here:
https://github.com/temporalio/helm-charts/blob/master/charts/temporal/templates/_helpers.tpl#L144-L159
like this:

{{- define "temporal.persistence.driver" -}}
    {{- $global := index . 0 -}}
    {{- $store := index . 1 -}} 
    {{- $storeConfig := index $global.Values.server.config.persistence $store -}}
    {{- if $storeConfig.driver -}}
        {{- $storeConfig.driver -}}
    {{- else if $global.Values.cassandra.enabled -}}
        {{- print "cassandra" -}}
    {{- else if $global.Values.mysql.enabled -}}
        {{- print "sql" -}}
    {{- else if $global.Values.postgresql.enabled -}}
        {{- print "sql" -}}
    {{- else -}}
        {{- required (printf "Please specify persistence driver for %s store" $store) $storeConfig.driver -}}
    {{- end -}}
{{- end -}}

what does {{- $store := index . 1 -}} get resolved into?
still, it looks like temporal.persistence.driver is a wrongly introduced key: there is no driver for 'persistence', there are only drivers for some stores, and since temporal basically has 3 possible stores (default + visibility, visibility-es)) - it should've instead been 3 different keys:
temporal.persistence.default.driver
temporal.persistence.visibility.driver
temporal.persistence.visibility_es.driver (I guess - is forbidden in the key paths)

[robholland]

This should be fixed now via #436