search

temporalio / helm-charts

Temporal Helm charts
MIT License
282 stars 316 forks source link

Fix condition for rendering visibility config #467

Closed np-we360 closed 4 months ago

np-we360 commented 4 months ago

While rendering visibility store config, instead of checking for .visibility key, it was checking for .default. Works when both your default and visibility store are same. Otherwise, doesn't.

What was changed

The condition for rendering visibility config in the server configmap was fixed.

Why?

This was a typo-like bug that was fixed.

Checklist

  1. Closes #466 .

  2. How was this tested: By rendering using helm template.

  3. Any docs updates needed? No.

CLAassistant commented 4 months ago

CLA assistant check
All committers have signed the CLA.

RajAnthari commented 4 months ago

Maybe #459 missed to remove Cassandra visibility changes from values.yaml as well.

image

I'm not sure whether it can be fixed with this PR or requires a new PR.

np-we360 commented 4 months ago

I think that's because Cassandra as a visibility store has been deprecated. I could be wrong though.

That said, I just added this deleted chunk on my local "fork" of this chart and it's working for now.

On this note, if Cassandra is truly deprecated I think we should get Helm to throw an error instead of continuing and producing a configuration that doesn't work at runtime.