feat(Makefile): containerize dev workflow#1049
feat(Makefile): containerize dev workflow#1049Joshua-Anderson wants to merge 1 commit intodeis:masterfrom
Conversation
|
@mboersma, @bacongobbler and @sgoings are potential reviewers of this pull request based on my analysis of |
|
Nice! Very excited for this.
@Joshua-Anderson first thought would be to have a separate target for the actual unit test command(s), say, |
|
Why is the |
|
@helgi The main difference in the two is that this one has dev-dependencies installed and doesn't expose any ports. Any ideas on how to combine the two? |
@vdice but if the unit tests fail, postgres wouldn't be stopped, would it? |
No idea tbh - Having 2 |
|
This solves the problem of needing to have the right python version installed, all the dependencies, and python configured to talk to a postgres database you need to install and run. This sort of stuff can be a big hurdle for community contributors and people like me who don't want to spend 30 minutes getting my computer properly configured. Now a developer can just run You're right that it doesn't solve the problem of running a particular test target. However, you can copy-paste the docker command from the makefile and edit it, or run the tests locally, since most core developers like you already have the environment setup. |
|
Again, not a fan of this. However if this is to be done I want a clearer distinction between the |
|
I'm not a fan of two Dockerfiles either. We should repurpose the Dockerfile in rootfs. If we are forced to go the two Dockerfile path, then perhaps a |
@Joshua-Anderson good point; although it would involve a bit of duplicated |
5a7adf2 to
ef2f14c
Compare
|
I updated the test-unit targets to automatically start/stop the database. I moved the main rootfs Dockerfile to be the root dockerfile and renamed the development dockerfile to @helgi and I talked and we're thinking the longer term solution is to update docker-python-dev and use that as a base for the test image. |
|
I tested |
ef2f14c to
a18eba7
Compare
Makefile
Outdated
| && coverage run manage.py test --settings=api.settings.testing --noinput registry api scheduler.tests \ | ||
| && coverage report -m | ||
| test-unit: start-postgres do-test-unit stop-postgres | ||
| do-test-unit: docker-build-dev |
There was a problem hiding this comment.
I'd propose we remove the docker-build-dev dependency here (as, if it fails, the deispg stop logic won't be executed) and place it into the above test-unit target, to look like:
test-unit: docker-build-dev start-postgres do-test-unit stop-postgres
do-test-unit:
...
(and same for the -quick variants below)
b478153 to
cd71204
Compare
|
@vdice Fixed! |
|
I'm not a regular controller contributor or maintainer, but as regards the Makefile/Dockerfile code here, LGTM, on top of the huge ease-of-use gains both for devs/contributors and ci. Tested manually via running |
|
One more thought @Joshua-Anderson -- should we perhaps update the |
| ln -s /usr/bin/python3 /usr/bin/python && \ | ||
| curl -sSL https://bootstrap.pypa.io/get-pip.py | python - pip==8.1.2 && \ | ||
| mkdir -p /configs && chown -R deis:deis /configs && \ | ||
| apt-get clean && \ |
There was a problem hiding this comment.
Yes, because you have to cleanup in the same layer you add the files. Deleting files in a different layer would make the image larger.
2c9f0cc to
5dbdfeb
Compare
ef36d76 to
d74c4c8
Compare
d74c4c8 to
d2e1218
Compare
|
I got this almost all working, but it's having codecov issues. If anyone is interested in going the last mile 😄 all deis maintainers have permission to push to this PR branch (thanks to a new GH feature), so you can update it. |
|
@Joshua-Anderson what's wrong with the codecov? I see it got posted in the travis build but also didn't see it post to GH. |
|
@helgi even though I specifically selected the file no test coverage reports were found. |
|
@Joshua-Anderson curious... the CLI system is having problems talking to codecov so that was my first instinct (that it was identical problems) |
|
Leaving this here http://docs.codecov.io/docs/testing-with-docker if anyone wants to take a crack at this |
|
Shall we punt this to another release or is someone planning on picking this back up this week? |
Fixes #18
Right now you have to manually start and stop postgres, but hopefully that can be improved so we don't have to in the future.
Does anyone know how to always run the targetmake stop-postgresaftermake test-unit?