search

YunoHost-Apps / Experimental_helpers

6 stars 12 forks source link

Add ynh_redis_get_free_db ynh_redis_remove_db #23

Closed Jibec closed 4 years ago

Jibec commented 6 years ago

I noticed no helper existed for redis to select the database. It also removes previous data.

I assume if I remove the database shared with another app, it may lead to nefast side-effects.

Here is an example with discourse: https://github.com/discourse/discourse/blob/master/config/discourse_defaults.conf#L115 https://github.com/YunoHost-Apps/discourse_ynh/blob/master/scripts/install#L130

JimboJoe commented 6 years ago

Well, if we don't remove the database, we're risking reaching the database limit one or another, aren't we? :thinking: Provided almost every application just uses default 0 database, my opinion is that the occurrence of a "shared" redis database is negligible...

Jibec commented 6 years ago

Not cleaning is bad ;) No we can't do that

I would rather detect packages with redis and fix them for future installs. Nothing can be done automatically for already installed once..

For example, peertube don't even allow to select redis database. This will be part of next release. -- Jean-Baptiste Holcroft

JimboJoe commented 6 years ago

I'm not sure to understand: you currently use flushall, which flushes the specified database and doesn't delete it... In my opinion we should delete it. Current applications never specify the database number, so they most probably end up using database 0. So, as these helpers won't allocate database 0 (which isn't available), we're most probably fine deleting the database, hence preventing reaching the database limit (at least for bad reasons)...

JimboJoe commented 6 years ago

Shouldn't we backup/restore the redis database as well...?

Jibec commented 6 years ago

I don't know if we really should backup-restore a redis database. It really doesn't looks like complicated: http://zdk.blinkenshell.org/redis-backup-and-restore/

Isn't redis only used for caching purposes? It isn't an issue for now, but I never had to test real restoration, only tests.

Btw, I can't understand what you said about the removing, what command do you suggest?

Jibec commented 6 years ago

Ok, funkwhale's developer suggest to backup the database. But it isn't mandatory, it's sounds like a good practice more than a must have. So I'll add this in the future.

JimboJoe commented 6 years ago

Well, sorry for my dumb remarks on database removal: I was naively considering there was a DROP kind of command... whereas FLUSHALL seems to somewhat "free" the database in the INFO keyspacecommand. I should have tested it first... :innocent:

I'm just wondering if we should use quotes when assigning the value to result, as it's a multi-line result...

For the database backup/restore, I think it should be done as well. It will just be tricky because we'll have to look for a free database (the one used priory could be used by another application), and thus modify the application configuration file... What do you think?