Closed caduvieira closed 7 years ago
coveralls
commented
7 years ago Coverage remained the same at 98.045% when pulling 58ccad07510124d60bae87a742519a030c4fabaa on caduvieira:docker_alpine into c599dc73298fdc5c1ceb4077fa1553c75358e90c on datasciencebr:master.
cuducos
commented
7 years ago The other is the Dockefile linter: how does it work, can you give us more information on that, on the advantages of using it?
Hell yeah, @caduvieira — just did my homework and Dockerfile linters are awesome : ) Just a minor change in requirements.txt and we're good to go!
anaschwendler
commented
7 years ago Hi @caduvieira and @cuducos. I've just merged the smaller docker image (I was imagining that was better after this PR). I'll test this one locally, and give you a better feedback :)
anaschwendler
commented
7 years ago So, me again here:
For me it is working ok @cuducos:
and the tests:
$ docker run --rm -v /tmp/serenata-data:/tmp/serenata-data rosie test➜ rosie git:(caduvieira-docker_alpine) docker run --rm -v /tmp/serenata-data:/tmp/serenata-data rosie test
......................................................................
----------------------------------------------------------------------
Ran 70 tests in 2.768s
OK@cuducos can you explain more to me about the problem?
caduvieira
commented
7 years ago @cuducos I couldn't reproduce that error
lipemorais
commented
7 years ago It took a long time to build the image but ran smoothly here too. :)
lipemorais
commented
7 years ago Hey @cuducos What do you think have this image builded by Docker hub?
lipemorais
commented
7 years ago Hey @cuducos What do you think have this image builded by Docker hub?
Looks that it's already done here so the part about how to run this image could be just the second line with a small modification on README.md something like docker run --rm -v /tmp/serenata-data:/tmp/serenata-data datasciencebr/rosie test since Docker downloads the image if it's not there yet.
@caduvieira @cuducos @anaschwendler What do you think of this?
cuducos
commented
7 years ago Ok, just re-ran the build and it worked — probably just some minor internet instability or whatever. I'm sorry about that.
I think we need small tweaks before I'm able to merge it:
During the scikit-learn wheel build it was failing because of "missing scipy"
So this is an intended exception. Could you document it adding a comment in the requirements.txt file? (BTW I tested sorting it and indeed it fails! Thanks for the catch, @caduvieira!)
a small modification on
README.mdsomething likedocker run --rm -v /tmp/serenata-data:/tmp/serenata-data datasciencebr/rosie testsince Docker downloads the image if it's not there yet.
And this edit suggested by @lipemorais, since people can use the image from Docker Hub instead of building theirs themselves ; )
coveralls
commented
7 years ago Coverage remained the same at 98.045% when pulling 00129b7e24b784dcd188f93f8a72aa54a2163afd on caduvieira:docker_alpine into c599dc73298fdc5c1ceb4077fa1553c75358e90c on datasciencebr:master.
caduvieira
commented
7 years ago Done and rebased
coveralls
commented
7 years ago Coverage remained the same at 98.045% when pulling cf817242c4e5d886fd3b60fdc0b80e8ab8659533 on caduvieira:docker_alpine into 49c069d674f3399f9b69c5c4c6b0ef81cd4fb21c on datasciencebr:master.
lipemorais
commented
7 years ago @caduvieira, may in the future we could split the new docker file based on Alpine and the docker file linter improvement in two different PR, so it could be merged even if the other one is not able to be merged yet. ;)
anaschwendler
commented
7 years ago Last review!
Clone the project:
$ git clone git@github.com:datasciencebr/rosie.gitChange to Rosie' folder:
$ cd rosieChange to the tested branch:
$ git checkout -b caduvieira-docker_alpine masterMerge its content:
$ git pull https://github.com/caduvieira/rosie.git docker_alpineRun the new Docker commands:
$ docker build -t rosie .
$ docker run --rm -v /tmp/serenata-data:/tmp/serenata-data rosie testThe result:
➜ rosie git:(caduvieira-docker_alpine) docker run --rm -v /tmp/serenata-data:/tmp/serenata-data rosie test
......................................................................
----------------------------------------------------------------------
Ran 70 tests in 3.189s
OKDone!
What is the purpose of this Pull Request? Using alpine as base image. Generate a smaller docker image. From 707 M to 381M Add linter to Dockerfile
What was done to achieve this purpose? Change the order of pip install
How to test if it really works? docker build -t rosie . docker run --rm -v /tmp/serenata-data:/tmp/serenata-data rosie test
Who can help reviewing it? Anyone with docker