ddev / ddev-redis

Redis service for DDEV
Apache License 2.0
23 stars 8 forks source link

Implemented redis persistence #18

Closed seebeen closed 1 year ago

seebeen commented 1 year ago

The Issue

1

How This PR Solves The Issue

Implements an environment variable to specify redis version Implements additional changes as discussed in the issue and on the Discord server

Automated Testing Overview

Just run the bats test

Release/Deployment Notes

PR merge will automatically trigger a semantic release process on the main branch and release version 2.0

seebeen commented 1 year ago
mkdir test-ddev-project && cd test-ddev-project && ddev config --auto --omit-containers=db && git clone https://github.com/oblakstudio/ddev-redis && ddev get ddev-redis

Installing project-level components: 
Unable to expand files and directories: stat commands/redis/redis-cli: no such file or directory

https://github.com/oblakstudio/ddev-redis-7

Wrong repo my man :)

Edit: My bad, I didn't change the name in the install.yaml. Fixed

stasadev commented 1 year ago

You forgot about redis-flush command.

And asking about this one again, the question does not go away if you resolve the conversation:

What if someone just wants to use 6 or 7-bookworm tag, let's give them this ability: before: image: redis:${DDEV_REDIS_VERSION:-6}-alpine after: image: redis:${DDEV_REDIS_VERSION:-6-alpine}

seebeen commented 1 year ago

You forgot about redis-flush command.

And asking about this one again, the question does not go away if you resolve the conversation:

What if someone just wants to use 6 or 7-bookworm tag, let's give them this ability: before: image: redis:${DDEV_REDIS_VERSION:-6}-alpine after: image: redis:${DDEV_REDIS_VERSION:-6-alpine}

I've applied your commit in the convo. Let me repush if it wasn't changed. But I'm thinking about selection of version and tag also

stasadev commented 1 year ago

Let me repush if it wasn't changed.

Yes, please push, there was no change for the image.

But I'm thinking about selection of version and tag also

Isn't that the same thing?

seebeen commented 1 year ago

Pushed

seebeen commented 1 year ago

I think the working directory should be changed from /data to something else.

Because there is an error:

$ ddev redis KEYS '*'
+ redis-cli -p 6379 -h redis -a redis --no-auth-warning KEYS appendonly.aof test-ddev-project.rdb
(error) ERR wrong number of arguments for 'keys' command

$ ddev exec -s redis "pwd && ls"
/data
appendonly.aof         test-ddev-project.rdb

In the regular ddev-redis /data is empty and the command is working:

$ ddev redis-cli KEYS '*'
(empty array)

Sorry didn't understand this. Workdir should be data, datadir should be /data. Where is the issue?

> + redis-cli -p 6379 -h redis -a redis --no-auth-warning KEYS appendonly.aof test-ddev-project.rdb

Why are you adding these two flags after KEYS command?

stasadev commented 1 year ago

Why are you adding these two flags after KEYS command?

I didn't add anything, * glob was expanded by sh inside the container.

I tested it again, and * can be expanded inside (sh) and outside (bash, zsh, etc.) the container.

What I meant is ddev redis KEYS * command in the example can have unexpected results depending on which folder it is run from.

For example, if you add two folders in the project mkdir test1 test2 and then run

ddev redis KEYS *
+ redis-cli -p 6379 -h redis -a redis --no-auth-warning KEYS test1 test2
(error) ERR wrong number of arguments for 'keys' command

* is expanded to test1 test2 outside the container.

Note that I run only ddev redis KEYS *, the second line + redis-cli ... is just an explanation of what is running (added with set -x into the script).

After this, when you run the command again with escaped *

ddev redis KEYS "*"
+ redis-cli -p 6379 -h redis -a redis --no-auth-warning KEYS appendonly.aof
(empty array)

* is expanded to appendonly.aof inside the container.

And looks like it works. But when you run ddev redis-flush, *.rdb file is added to /data:

ddev redis KEYS "*"
+ redis-cli -p 6379 -h redis -a redis --no-auth-warning KEYS appendonly.aof test-ddev-project.rdb
(error) ERR wrong number of arguments for 'keys' command

The only way I found how to prevent expansion, is to escape * two times:

ddev redis KEYS "'*'"
+ redis-cli -p 6379 -h redis -a redis --no-auth-warning KEYS '*'
(empty array)

I updated the example.

seebeen commented 1 year ago

Since we mutually decided to split the plugins. I'm closing this PR