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 condition for rendering visibility config #467

Closed
wants to merge 1 commit into from
Closed

Fix condition for rendering visibility config #467

wants to merge 1 commit into from

Conversation

np-we360
Copy link

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 Visibility config doesn't get rendered if visibility and default store types are different #466 .

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

  3. Any docs updates needed?
    No.

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.
@np-we360 np-we360 requested review from a team as code owners February 19, 2024 10:24
@CLAassistant
Copy link

CLAassistant commented Feb 19, 2024

CLA assistant check
All committers have signed the CLA.

@RajAnthari
Copy link

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
Copy link
Author

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.

@np-we360 np-we360 closed this by deleting the head repository Mar 16, 2024
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.

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