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

Implemented redis persistence #18

Closed
wants to merge 16 commits into from
Closed

Implemented redis persistence #18

wants to merge 16 commits into from

Conversation

seebeen
Copy link

@seebeen seebeen commented Sep 20, 2023

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

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
.github/workflows/reusable_test_ddev_addon.yml Outdated Show resolved Hide resolved
docker-compose.redis.yaml Outdated Show resolved Hide resolved
.github/workflows/reusable_test_ddev_addon.yml Outdated Show resolved Hide resolved
Copy link
Author

@seebeen seebeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes implemented

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
images/anon-volume.jpg Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
.github/workflows/reusable_test_ddev_addon.yml Outdated Show resolved Hide resolved
docker-compose.redis.yaml Outdated Show resolved Hide resolved
Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@seebeen
Copy link
Author

seebeen commented Sep 26, 2023

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
Copy link
Member

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
Copy link
Author

seebeen commented Sep 26, 2023

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
Copy link
Member

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
Copy link
Author

seebeen commented Sep 26, 2023

Pushed

Copy link
Member

@stasadev stasadev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Comment on lines +3 to +7
## Description: Run redis-cli inside the redis container
## Usage: redis-flush
## Example: "redis-flush"

redis-cli -a redis --no-auth-warning FLUSHALL ASYNC
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## Description: Run redis-cli inside the redis container
## Usage: redis-flush
## Example: "redis-flush"
redis-cli -a redis --no-auth-warning FLUSHALL ASYNC
## Description: Clear all Redis databases
## Usage: redis-flush
## Example: "ddev redis-flush"
set -x
redis-cli -a redis --no-auth-warning FLUSHALL ASYNC

Fixed the command description and example.
Redis commands are so complicated, let's add set -x to see what is actually executing.

Comment on lines +3 to +7
## Description: Run redis-cli inside the redis container
## Usage: redis [flags] [args]
## Example: "redis KEYS *" or "ddev redis-cli INFO" or "ddev redis-cli --version"

redis-cli -p 6379 -h redis -a redis --no-auth-warning "$@"
Copy link
Member

@stasadev stasadev Sep 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
## Description: Run redis-cli inside the redis container
## Usage: redis [flags] [args]
## Example: "redis KEYS *" or "ddev redis-cli INFO" or "ddev redis-cli --version"
redis-cli -p 6379 -h redis -a redis --no-auth-warning "$@"
## Description: Run redis-cli inside the redis container
## Usage: redis [flags] [args]
## Example: ddev redis KEYS "'*'" or "ddev redis INFO" or "ddev redis --version"
set -x
redis-cli -p 6379 -h redis -a redis --no-auth-warning "$@"

Fixed the command examples.
Quotes around '*' - because it is expanded when running from zsh.
Redis commands are so complicated, let's add set -x to see what is actually executing.

@seebeen
Copy link
Author

seebeen commented Sep 27, 2023

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
Copy link
Member

stasadev commented Sep 27, 2023

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
Copy link
Author

seebeen commented Sep 27, 2023

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

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.

2 participants