ci: use locally-installed redis instead of dockerized one #113
Conversation
|
@tumile I modified the CI workflow to use the locally-installed Redis instead of the dockerized one. |
uki00a
changed the title
| - name: Install and start Redis | ||
| run: | | ||
| make testdeps | ||
| make start-redis |
There was a problem hiding this comment.
Do we need to start redis here? I think this can be handled by the test suite with startRedisServer. It's probably better to do it in code, we may have more control over the configuration.
There was a problem hiding this comment.
It's because I prioritized making testing easier in a local environment.
Some people might already run Redis locally, but others might do in a Docker container.
There was a problem hiding this comment.
Ummm, but I also don't want the test to fail because of environmental differences.
Thinking so, using startRedisServer isn't a bad idea... 🤔
There was a problem hiding this comment.
Yeah, I was also thinking what if the redis-server instance already running on a user's machine have some conflicting configs like cluster-enabled for example, it will break normal tests
There was a problem hiding this comment.
OK, I'll change the tests later to use startRedisServer to start the testing Redis.
| ${TESTDATA_DIR}/redis/src/redis-server --port 6379 --daemonize yes | ||
|
|
||
| kill-redis: | ||
| ${TESTDATA_DIR}/redis/src/redis-cli -p 6379 shutdown |
There was a problem hiding this comment.
Same here, I think it's more versatile to start and kill redis-server in code
| redis: | ||
| docker run -p 6379:6379 -d -t redis:5 |
There was a problem hiding this comment.
Is this still relevant now that we use a local instance of redis?
|
Oops, I see that this has already been merged 😅. I left a couple of thoughts though, hope you can take a look. |
Towards #104 and #110.