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

Adding support for DBINDEX redis config value in docker compose #2288

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

GroondCharge
Copy link

Currently, there is no support for adding dbindex config value ($CONFIG['redis']['dbindex']) within compose file. Even in manually updating or setting the value for a container, the changes aren't applied. This fixes the missing option.

@joshtrichards
Copy link
Member

Please see:

#2236
#1286

I'm not 100% against this, but the session handler needs to be considered too. It's not clear what the "right thing to do here is".

Personally I'd just avoid dbindex and use dedicated Redis. This is a container environment after all. :-)

@GroondCharge
Copy link
Author

Please see:

#2236 #1286

I'm not 100% against this, but the session handler needs to be considered too. It's not clear what the "right thing to do here is".

Personally I'd just avoid dbindex and use dedicated Redis. This is a container environment after all. :-)

#1286 (comment) this becomes a hassle especially, when one'd try to automate deployment. It feels like this would be a nice feature to have available if needed irregardless if it's not optimal for 90% of the cases. However! There is a middle ground imo - prefixes. This would work if you had a redis instance that's running for multiple services or even, if there are multiple NCs connecting to a redis cluster

@GroondCharge
Copy link
Author

According to this: nextcloud/server#32203 the prefix for nextcloud is calculated, but then changes when either path, version and/or instanceid would change. If there was a way to make prefix optionally persistent, then these issues would be solved.

@szaimen
Copy link
Contributor

szaimen commented Sep 4, 2024

Hi, maybe this helps as reference?
nextcloud/all-in-one#5126

Changing naming convetion to match others

Signed-off-by: Jan Zalar <[email protected]>
@GroondCharge
Copy link
Author

Hi, maybe this helps as reference? nextcloud/all-in-one#5126

Thank you for this, i changed it to configure session locking appropriately, aswell as name for env variable, to match the ones you used

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.

4 participants