Add config fallbacks for session, cache and queue when disabling redis-cluster #585

2023-12-11T16:31:35Z

pat-s commented

2023-12-11 16:31:35 +00:00

(Migrated from gitea.com)

Description of the change

Add config fallbacks for session, cache and queue including tests.

Benefits

If users disable the default redis-cluster sub-chart dependency, this will configure the respective sections to use the Gitea defaults as listed in https://docs.gitea.com/next/administration/config-cheat-sheet.

Possible drawbacks

Users will run on non-optimal settings for production without knowing their config.

Applicable issues

fixes #584 #573 #489 #476 #468 #453

Checklist

Parameters are documented in the values.yaml and added to the README.md using readme-generator-for-helm
Breaking changes are documented in the README.md
Templating unittests are added

### Description of the change Add config fallbacks for `session`, `cache` and `queue` including tests. ### Benefits If users disable the default `redis-cluster` sub-chart dependency, this will configure the respective sections to use the Gitea defaults as listed in https://docs.gitea.com/next/administration/config-cheat-sheet. ### Possible drawbacks Users will run on non-optimal settings for production without knowing their config. ### Applicable issues - fixes #584 #573 #489 #476 #468 #453 ### Checklist  - [x] Parameters are documented in the `values.yaml` and added to the `README.md` using [readme-generator-for-helm](https://github.com/bitnami-labs/readme-generator-for-helm) - [x] Breaking changes are documented in the `README.md` - [x] Templating unittests are added

justusbunsi (Migrated from gitea.com) reviewed 2023-12-11 16:31:45 +00:00

justusbunsi commented

2023-12-11 17:39:08 +00:00

(Migrated from gitea.com)

Without having enough time to review this today, a suggestion regarding Users will run on non-optimal settings for production without knowing their config.:

We could print a notice within https://gitea.com/gitea/helm-chart/src/branch/main/templates/NOTES.txt. This is shown when running helm install or helm upgrade. That's the only possible way to let users know that their configuration might not be best fit for production while not cancelling the helm templating. What do you think about that, @pat-s?

Without having enough time to review this today, a suggestion regarding `Users will run on non-optimal settings for production without knowing their config.`: We could print a notice within https://gitea.com/gitea/helm-chart/src/branch/main/templates/NOTES.txt. This is shown when running `helm install` or `helm upgrade`. That's the only possible way to let users know that their configuration might not be best fit for production while not cancelling the helm templating. What do you think about that, @pat-s?

pat-s commented

2023-12-12 14:12:38 +00:00

(Migrated from gitea.com)

Might surely "better than nothing" but helm charts are often wrapped by other tools and such hints don't appear there (terraform, argo (?)). I guess the best we can do is to highlight it in the README (in addition to NOTES.txt, surely a good idea) and "just hope for the best". At some point users are also just solely responsible for their settings and might notice when things aren't running smooth.

justusbunsi commented

2023-12-12 17:44:33 +00:00

(Migrated from gitea.com)

Might surely "better than nothing" but helm charts are often wrapped by other tools and such hints don't appear there (terraform, argo (?)). I guess the best we can do is to highlight it in the README (in addition to NOTES.txt, surely a good idea) and "just hope for the best". At some point users are also just solely responsible for their settings and might notice when things aren't running smooth.

Ah, right. Wrapper tools hide those notes. I like your suggestion to add it to the readme as well. Let's do both, as you said. This should™️ help.

> Might surely "better than nothing" but helm charts are often wrapped by other tools and such hints don't appear there (terraform, argo (?)). I guess the best we can do is to highlight it in the README (in addition to NOTES.txt, surely a good idea) and "just hope for the best". At some point users are also just solely responsible for their settings and might notice when things aren't running smooth. Ah, right. Wrapper tools hide those notes. I like your suggestion to add it to the readme as well. Let's do both, as you said. This should™️ help.

pat-s commented

2023-12-13 09:36:23 +00:00

(Migrated from gitea.com)

@justusbunsi Added both. Feel free to modify directly if you have suggestions to content and wording.

justusbunsi commented

2023-12-15 20:36:11 +00:00

(Migrated from gitea.com)

Oh dear, I really tried to break your changes. 😆

As you can see, I changed a few things. My commit messages should be descriptive. If not, let me know.
Beside those changes, I could not find any issues regarding your changes. Although, I found unrelated things (see second part of this comment).

Having "redis-cluster" disabled creates this tiny invalid block inside app.ini. So in theory it would've been enough to not set these settings while "redis-cluster" is disabled.

[session]
PROVIDER_CONFIG =
PROVIDER = redis

But I really appreciate what you've did here:
Explicitly set Gitea internal defaults (if not otherwise defined) is a neat way to reduce the current impact of #356 for users experimenting with/without Redis.
As soon as that issue is resolved, we should be able to drop these explicit resets again and simplify our code base. I've added a note in #356.

While reviewing this PR, I've also noticed that unless "redis-cluster" is completely disabled, users will never be able to overrule our redis related defaults via inline values. Neither with the current main state, nor with the changes in this PR.
Sure, they can overload them via additionalConfigSources - they are independent from inline values. But still have an inconsistent app.ini.
That explains the user complaints about redis being very hard to overwrite.

Imagine: Trying to use "redis-cluster" for everything except for sessions where db is desired.
Our defaults will prepare [session].PROVIDER_CONFIG for "redis-cluster". Per documentation, that option should be empty when [session].PROVIDER=db.
Luckily, it doesn't conflict and sessions are correctly written into the database. But it is at least confusing for admins looking at their Gitea config.
As this issue most likely exists since the introduction of additionalConfigSources, I consider this out of scope of this PR 🙂. But without this PR I wouldn't have noticed.
We need to reconsider the whole app.ini creation with #356 anyway, so it might be a good fit there. I've added a note about this finding over there, too.

Oh dear, I really tried to break your changes. :laughing: As you can see, I changed a few things. My commit messages should be descriptive. If not, let me know. Beside those changes, I could not find any issues regarding your changes. Although, I found unrelated things (see second part of this comment). Having "redis-cluster" disabled creates this tiny invalid block inside `app.ini`. So in theory it would've been enough to not set these settings while "redis-cluster" is disabled. ```text [session] PROVIDER_CONFIG = PROVIDER = redis ``` **But I really appreciate what you've did here:** Explicitly set Gitea internal defaults (if not otherwise defined) is a neat way to reduce the current impact of #356 for users experimenting with/without Redis. As soon as that issue is resolved, we should be able to drop these explicit resets again and simplify our code base. I've added a note in #356. --- While reviewing this PR, I've also noticed that unless "redis-cluster" is completely disabled, users will never be able to overrule our redis related defaults via inline values. Neither with the current main state, nor with the changes in this PR. Sure, they _can_ overload them via `additionalConfigSources` - they are independent from inline values. But still have an inconsistent app.ini. That explains the user complaints about redis being _very_ hard to overwrite. **Imagine:** Trying to use "redis-cluster" for everything except for sessions where `db` is desired. Our defaults will prepare `[session].PROVIDER_CONFIG` for "redis-cluster". [Per documentation](https://docs.gitea.com/next/administration/config-cheat-sheet#session-session), that option should be empty when `[session].PROVIDER=db`. Luckily, it doesn't conflict and sessions are correctly written into the database. But it is at least confusing for admins looking at their Gitea config. As this issue most likely exists since the introduction of `additionalConfigSources`, I consider this out of scope of this PR :slightly_smiling_face:. But without this PR I wouldn't have noticed. We need to reconsider the whole app.ini creation with #356 anyway, so it might be a good fit there. I've added a note about this finding over there, too.

justusbunsi (Migrated from gitea.com) approved these changes 2023-12-15 20:37:26 +00:00

justusbunsi (Migrated from gitea.com) left a comment

LGTM. I keep it up to you to merge this PR. That way you have the chance to review my changes.

pat-s commented

2023-12-18 08:41:56 +00:00

(Migrated from gitea.com)

Highly appreciated and very good findings! If every PR would be reviewed with your standards, I'd be very happy :)

Having "redis-cluster" disabled creates this tiny invalid block inside app.ini.

Yes, this has been a "bug" beforehand I'd say. I'd argue that it's not the actual reason why users faced problems but it definitely contributed to it.

Explicitly set Gitea internal defaults (if not otherwise defined)

I thought for some time about it as it comes back to the idea of "going with application defaults even though they are not well suited" which is and will always be a debatable topic: I.e., how opinionated should a chart be in terms of the default and general configuration (of course allowing everything but WRT to it's own template actions).

Trying to use "redis-cluster" for everything except for sessions where db is desired.

Yes I see. Though maybe this use case never applied so far as either users used redis for all of the three or for none. But yes, in theory this should be possible without duplicated config sections.

A general test would be great which issues a warning if duplicated config sections/mappings are detected.

I also think that this is an improvement for now and we can go with it (and also release 1.21.2). Thanks also for the missed bracket in NOTES.txt!

Highly appreciated and very good findings! If every PR would be reviewed with your standards, I'd be very happy :) > Having "redis-cluster" disabled creates this tiny invalid block inside app.ini. Yes, this has been a "bug" beforehand I'd say. I'd argue that it's not the actual reason why users faced problems but it definitely contributed to it. > Explicitly set Gitea internal defaults (if not otherwise defined) I thought for some time about it as it comes back to the idea of "going with application defaults even though they are not well suited" which is and will always be a debatable topic: I.e., how opinionated should a chart be in terms of the default and general configuration (of course allowing everything but WRT to it's own template actions). > Trying to use "redis-cluster" for everything except for sessions where db is desired. Yes I see. Though maybe this use case never applied so far as either users used redis for all of the three or for none. But yes, in theory this should be possible without duplicated config sections. A general test would be great which issues a warning if duplicated config sections/mappings are detected. I also think that this is an improvement for now and we can go with it (and also release 1.21.2). Thanks also for the missed bracket in `NOTES.txt`!

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: lunny/helm-chart#585

Add config fallbacks for session, cache and queue when disabling redis-cluster #585

Description of the change

Benefits

Possible drawbacks

Applicable issues

Checklist

Add config fallbacks for `session`, `cache` and `queue` when disabling redis-cluster #585