Dockerize #1365
Dockerize #1365
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1365 +/- ##
==========================================
- Coverage 92.76% 85.43% -7.33%
==========================================
Files 20 7 -13
Lines 6581 3008 -3573
==========================================
- Hits 6105 2570 -3535
+ Misses 476 438 -38
Continue to review full report at Codecov.
|
|
@RoeyPrat Do you anticipate wanting a "debug" or "test" type of container that has the redis-py code -- for manual or automated testing purposes? In my local testing with this setup, I've been running |
|
I think it would be nice to provide a container that executes the tox suite. If a contributor only needs docker installed to execute the full test suite, that seems like a significant improvement. |
|
Also, it would be really nice if we can specify the source Redis docker image in a single location. Perhaps we could have a multistage build with a single Dockerfile that references the official Redis docker image. All the other Dockerfiles that you have now can be based on the intermediate one. This way if we want to upgrade to Redis 6.1, we would only need to change a that one intermediate file. |
|
Good point! I’ll make those changes. -A
…On Wed, Jul 8, 2020 at 5:37 PM Andy McCurdy ***@***.***> wrote:
Also, it would be really nice if we can specify the source Redis docker
image in a single location. Perhaps we could have a multistage build with a
single Dockerfile that references the official Redis docker image. All the
other Dockerfiles that you have now can be based on the intermediate one.
This way if we want to upgrade to Redis 6.1, we would only need to change a
that one intermediate file.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1365 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAXXHSJSHBRTQXRKPSFZHDR2UGK7ANCNFSM4OU3WEMQ>
.
|
|
@andymccurdy Hey, Andy! I read through the docs on multi-stage builds, and they all seemed to refer to situations a little different from this one. In the end I took the approach of building a base image that all the Dockerfiles use, pointed at the "latest" tag. So we would bump the base build and then rebuild the other images, which would pull the new base image. What do you think about that? I also made some test changes to handle the fact that, in a mult-container setup, each container has its own hostname and IP. Some of these commands get pretty complex, so I added a Makefile to contain them. |
Makefile
Outdated
|
|
||
| build: | ||
| docker build -t redis-py-base docker/base | ||
| docker-compose down |
There was a problem hiding this comment.
Not sure this line is really necessary.
tests/conftest.py
Outdated
| action="store", | ||
| help="Redis connection string," | ||
| " defaults to `%(default)s`") | ||
| parser.addoption('--redis-master-host', default=DEFAULT_REDIS_MASTER_HOST, |
There was a problem hiding this comment.
Is there ever a reason where redis-master-host would not point to the same host that's defined in the redis-url? I can't think of one.
Assuming there isn't a valid reason, we should infer the master hostname from the redis-url argument rather than introducing a new setting.
There was a problem hiding this comment.
Probably not. Fixed!
tests/conftest.py
Outdated
| @@ -156,6 +161,11 @@ def mock_cluster_resp_slaves(request, **kwargs): | |||
| return _gen_cluster_mock_resp(r, response) | |||
|
|
|||
|
|
|||
| @pytest.fixture(scope="module") | |||
There was a problem hiding this comment.
Does the master_host value ever change from module to module? If not, scoping this to 'session' seems more appropriate.
There was a problem hiding this comment.
It shouldn't, no - updated!
|
@andymccurdy What do you think about me adding a CONTRIBUTING.rst file, or a new section to README.rst, with a walk-through of how to set up your dev environment and run the tests? |
|
@abrookins That would be great. CONTRIBUTING sounds like the appropriate place. We can add a link in the README to the CONTRIBUTING page. |
|
Last remaining problem here is getting codecov to work -- I'm not sure what the right environment variable it's looking for is, and @andymccurdy had some ideas to make this work. |
docker-entry.sh
Outdated
| @@ -0,0 +1,17 @@ | |||
| #!/bin/sh | |||
|
|
|||
| # This is the entrypoint for "make clean". It invokes Tox. If running | |||
There was a problem hiding this comment.
Small nit - it's "make test" right?
|
It looks like coverage reduced partially because this test doesn't run: https://github.com/andymccurdy/redis-py/blob/master/tests/test_connection.py#L9
Hmm. |
|
codecov from tox seems to work! 🥳 I'm a little confused about how running coverage with multiple environments interacts with the "coverage" file. I'm trying some options to make sure that we're combining the output of all the runs when we eventually run |
|
Ok, this is better. Now we get a text-based coverage report when we run tests, and we produce the XML that We'll always produce the text report, so we can see it either on the terminal or in build output, but we'll only run |
Pull Request check-list
Please make sure to review and check all of these items:
$ toxpass with this change (including linting)?NOTE: these things are not required to open a PR and can be done
afterwards / while the PR is open.
Description of change
Move from using Vagrant to build and run a dev environment and tests to using Docker.