feat(Helm): Redis with password supported in helm charts and redis chart version updated

[wiktor2200]

SUMMARY

Redis with password was not supported out-of-the-box, so I've added this support in celery config in _helpers. When variable: redis_password in section: supersetNode.connections other version of config script is created during init.

I've also updated quite ancient version of Redis helm chart in dependencies.
Update process was performed according to this doc: https://artifacthub.io/packages/helm/bitnami/redis/16.3.1

I've also added small fix in documentation for helm chart. #18641

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

I've updated values.yaml json schema. helm lint command runs successfully. I've also checked helm templates if configs are generated without any missing/additional white space.

ADDITIONAL INFORMATION

• Has associated issue:
• Required feature flags:
• Changes UI
• Includes DB Migration (follow approval process in SIP-59)
  • Migration is atomic, supports rollback & is backwards-compatible
  • Confirm DB migration upgrade and downgrade tested
  • Runtime estimates and downtime expectations provided
• Introduces new feature or API
• Removes existing feature or API

[wiktor2200]

@ad-m Could you take a look on values.schema.json schema and review it?

@craig-rueda Could you take a look?

[ad-m]

Some lines are duplicated in both condition situations. I wonder if we want to have two ifs or if we want duplicate code. I think it's better not to have two ifs than duplicate code, because then it's easier to maintain consistency when a new parameter is added in the middle.

[craig-rueda]

I thought the same thing, but drying that up will be a little tricky and will likely make the code less readable in this case.

[ad-m]

I have in mind something like:

class CeleryConfig(object):
  CELERY_ANNOTATIONS = {'tasks.add': {'rate_limit': '10/s'}}
  CELERY_IMPORTS = ('superset.sql_lab', )
{{- if .Values.supersetNode.connections.redis_password }}
  CELERY_RESULT_BACKEND = f"redis://{env('REDIS_PASSWORD')}@{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"
  BROKER_URL = f"redis://{env('REDIS_PASSWORD')}@{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"
{{- else }}
  CELERY_RESULT_BACKEND = f"redis://{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"
  BROKER_URL = f"redis://{env('REDIS_HOST')}:{env('REDIS_PORT')}/0"
{{- end -}}

CELERY_CONFIG = CeleryConfig
RESULTS_BACKEND = RedisCache(
      host=env('REDIS_HOST'),
{{- if .Values.supersetNode.connections.redis_password }}
      password=env('REDIS_PASSWORD'),
{{- end -}}
      port=env('REDIS_PORT'),
      key_prefix='superset_results'
)

Do you think readability has been maintained?

[wiktor2200]

It makes sense. I've tried it this way before but I wanted diff to be minimal, so I didn't want to change order od parameters. I changed it, because I agree that when it will become bigger one of if/else/end could be omitted and cause lots of troubleshooting.

[wiktor2200]

Thanks for your feedback! And please take a look once again. @craig-rueda @ad-m

[wiktor2200]

Hello @craig-rueda something went wrong with promoting these changes as release.
I guess it was cause of merging this one and #18649 with the same release version.

[ad-m]

Hello @craig-rueda something went wrong with promoting these changes as release. I guess it was cause of merging this one and #18649 with the same release version.

Could you provide PR to bump version one more time?

[wiktor2200]

I've created this PR: #18751, please merge @craig-rueda.
@ad-m if this one will be merged it will affect your changes here: https://github.com/apache/superset/pull/18668/files#diff-23d50af2c34a57c2eef8579ccda92efbfc0fd81e189851948d9e2b556a5d45cc
so you will also need to bump version.

[yzliao-zip]

I think the check of auth.enabled is needed here. Otherwise, the redis without password is connected with an incorrect url because the default value in value.yaml is set.