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

pat-s · 2023-12-11T16:31:35Z

pat-s commented

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

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

pat-s added the

kind

enhancement

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

pat-s added 2 commits 2023-12-11 16:31:35 +00:00

session config fallbacks and tests 5040770464

add cache and queue config tests

check-and-test / check-and-test (pull_request) Failing after 30s

Details

39a04a409b

pat-s requested review from justusbunsi 2023-12-11 16:31:45 +00:00

pat-s added 1 commit 2023-12-11 16:33:13 +00:00

lint

check-and-test / check-and-test (pull_request) Successful in 30s

Details

a41e30a827

justusbunsi commented

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

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

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

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 added 2 commits 2023-12-13 09:36:00 +00:00

print warning in notes.txt for fallback config 95c063a964

add section to README

check-and-test / check-and-test (pull_request) Failing after 13s

Details

c5a5b91e9d

pat-s commented

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

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

pat-s added 1 commit 2023-12-13 09:39:21 +00:00

eq

check-and-test / check-and-test (pull_request) Successful in 31s

Details

860fc3b593

justusbunsi added 4 commits 2023-12-15 20:22:12 +00:00

Clean up [session].PROVIDER_CONFIG for disabled redis-cluster bdf36c9008

Signed-off-by: justusbunsi <sk.bunsenbrenner@gmail.com>

Fix/improve test descriptions 141cf08a03

Signed-off-by: justusbunsi <sk.bunsenbrenner@gmail.com>

Distinguish custom values in tests bf35498673

This helps to ensure that the correct value is stored in the correct
setting.

Signed-off-by: justusbunsi <sk.bunsenbrenner@gmail.com>

Fix NOTES rendering

check-and-test / check-and-test (pull_request) Successful in 48s

Details

575186e537

The warnings were printed in one single line.

Signed-off-by: justusbunsi <sk.bunsenbrenner@gmail.com>

justusbunsi referenced this pull request

2023-12-15 20:27:56 +00:00

Default values are not restored when removing custom values #356

justusbunsi added 1 commit 2023-12-15 20:30:24 +00:00

Merge branch 'main' into config-fallbacks

check-and-test / check-and-test (pull_request) Successful in 29s

Details

568a20b577

justusbunsi commented

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

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 approved these changes 2023-12-15 20:37:26 +00:00

justusbunsi 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

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`!

pat-s added 1 commit 2023-12-18 08:42:02 +00:00

Merge branch 'main' into config-fallbacks

check-and-test / check-and-test (pull_request) Successful in 33s

Details

7de789e157

pat-s merged commit 223069d042 into main

2023-12-18 08:43:19 +00:00

pat-s referenced this issue from a commit

2023-12-18 08:43:19 +00:00

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

pat-s deleted branch config-fallbacks

2023-12-18 08:43:21 +00:00