inveniosoftware / helm-invenio

Helm charts for deploying an Invenio instance
https://helm-invenio.readthedocs.io
7 stars 19 forks source link

redis: Replace old templates with Helm dependency #99

Closed lindhe closed 7 months ago

lindhe commented 7 months ago

Description

This change replaces the built in templates for Redis with the bitnami/redis chart included as a Helm dependency.

In order for the wipe_recreate.sh script to work, disableCommands had to be modified to allow FLUSHALL to run in Redis.

Checklist

Ticks in all boxes and 🟢 on all GitHub actions status checks are required to merge:

Third-party code

If you've added third-party code (copy/pasted or new dependencies), please reach out to an architect.

Reminder

By using GitHub, you have already agreed to the GitHub’s Terms of Service including that:

  1. You license your contribution under the same terms as the current repository’s license.
  2. You agree that you have the right to license your contribution under the current repository’s license.
plesoun-stein commented 7 months ago

bitnami's redis service name format

bitnami has service name format fullname-master for redis

where fullname is from common.names.fullname

so

redis service name should be like

{{- $redisServiceShortName := printf "%s-master" ( include "common.names.fullname" .Subcharts.redis ) }}
plesoun-stein commented 7 months ago

redis generates 10 chars passwords

may be good idea create extra secret with more secure password

and configure values.yaml to use your own instead of original

lindhe commented 7 months ago

redis generates 10 chars passwords

may be good idea create extra secret with more secure password

and configure values.yaml to use your own instead of original

Thanks for the input, @plesoun-stein! Since the old packaging of Redis used unauthenticated requests, I opted to keep it that way in this PR. I want to change as little as possible when replacing it, so it's easier to verify that the functionality is unchanged and to keep each PR minimal. I agree that authenticated requests would be a great addition, but I would prefer if that could go in a separate PR after this one. What do you think?

lindhe commented 7 months ago

bitnami's redis service name format

bitnami has service name format fullname-master for redis

where fullname is from common.names.fullname

so

redis service name should be like

{{- $redisServiceShortName := printf "%s-master" ( include "common.names.fullname" .Subcharts.redis ) }}

Is that not what I do here? Is something wrong that I don't see? If you put a comment on the specific line where you want a change, it gets easier to reason about it with the right context.

plesoun-stein commented 7 months ago

bitnami's redis service name format

bitnami has service name format fullname-master for redis where fullname is from common.names.fullname so redis service name should be like

{{- $redisServiceShortName := printf "%s-master" ( include "common.names.fullname" .Subcharts.redis ) }}

Is that not what I do here? Is something wrong that I don't see? If you put a comment on the specific line where you want a change, it gets easier to reason about it with the right context.

I was wrong. I'm sorry my mistake.

lindhe commented 7 months ago

I replied to your comment in the "Files changed" view. Not sure why the conversation does not show up here in the PR thread…