-
Notifications
You must be signed in to change notification settings - Fork 61
Conversation
I runned just So I added the I'm not sure if this is ok for production environment or can be changed based on environment. I don't understand why there are 3 and no just 2 Docker compose files, something like Talking about instructions to run the project with Docker on Readme. The sentence below is missing a
I was not able to run the production docker-compose using the command
|
Wow! Many many thanks for this awesome review : ) Let me try to clarify some parts and recognize my mistakes hahaha…
Good point: gonna add
There are things we might wan't on development environment but not in production. And also things I'm not sure how to unset in the hypothetical So the combination
I don't think it's missing a
Basically the problem is that now Docker uses the
In sum, three pending things to be committed for sure:
And I still would like to hear from you about the 3 files (if we keep |
cf70be9
to
ae8fc0d
Compare
As Elm tests were keeping this PR from going through I removed them by now and suggested (editing the opening post) to create an specific issue to update the test suite (basically move from @anaschwendler I addressed some of @lipemorais concerns, held back from other (to which @gomex reacted with a 👍 ) so I think we can move on to actually testing it and core reviewing. If you need help with the tests described in the opening post, LMK. |
Hey @cuducos, I'll be taking a look at this PR now! What I did to test this PR:
$ git clone [email protected]:datasciencebr/jarbas.git
$ cd jarbas
$ git checkout -b cuducos-production-docker origin/cuducos-production-docker
$ git merge master
$ cp contrib/.env.sample env
$ docker-compose up -d And works fine:
$ docker-compose run --rm django python manage.py migrate
$ docker-compose run --rm django python manage.py reimbursements /mnt/data/reimbursements_sample.xz
$ docker-compose run --rm django python manage.py companies /mnt/data/companies_sample.xz
$ docker-compose run --rm django python manage.py suspicions /mnt/data/suspicions_sample.xz
$ docker-compose run --rm django python manage.py tweets PS.: I'll be updating the comment as long as I keep it going |
README.md
Outdated
$ docker-compose run --rm jarbas python manage.py reimbursements /mnt/data/reimbursements_sample.xz | ||
$ docker-compose run --rm jarbas python manage.py companies /mnt/data/companies_sample.xz | ||
$ docker-compose run --rm jarbas python manage.py suspicions /mnt/data/suspicions_sample.xz | ||
$ docker-compose run --rm jarbas python manage.py tweets | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change jarbas
to django
:
$ docker-compose run --rm django python manage.py reimbursements /mnt/data/reimbursements_sample.xz
$ docker-compose run --rm django python manage.py companies /mnt/data/companies_sample.xz
$ docker-compose run --rm django python manage.py suspicions /mnt/data/suspicions_sample.xz
$ docker-compose run --rm django python manage.py tweets
Thanks, @anaschwendler — docs fixed in 7029a89 |
About the error on And would you mind sharing the logs and outputs? |
Development environment
I believe that this last line does not sounds good and have a relation with this issue.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that be able to see the elm code changes reflected is very important to make it able to be merged. :)
@@ -1,2 +0,0 @@ | |||
https://github.com/heroku/heroku-buildpack-nodejs.git | |||
https://github.com/heroku/heroku-buildpack-python.git |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are this build packs related to this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. We first launched Jarbas on Heroku and we considered using Dokku together with Digital Ocean. In both cases the .buildpacks
file was required. This PR intends to take Docker Machine as the offical way to deploy Jarbas — so the .builpacks
are nor necessary anymore.
README.md
Outdated
If you're interesting in having a database full of data you can get the datasets running [Rosie](https://github.com/datasciencebr/rosie). | ||
To add a fresh new `reimbursements.xz` or `suspicions.xz` brewed by [Rosie](https://github.com/datasciencebr/rosie), or a `companies.xz` you've got from the [toolbox](https://github.com/datasciencebr/serenata-toolbox), you just need copy these files to `contrib/data` and refer to them inside the container from the path `/mnt/data/`. | ||
|
||
#### Acessing Jabas |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo Jabas
Definitively this was not supposed to happen. Can you share the logs for this container? Why am I saying this definetively wasn't supose to happen IMHO:
So I guess there is an error in the conatiner. And that's why I ask for the logs ; ) |
Would you mind if I ask you to put something in the installation like we have in Oh @lipemorais already sent the logs, faster than me hehe |
@cuducos Here are the all logs when I run it.
|
@anaschwendler, can you be more specific?
The @lipemorais: can you rebuild the Elm container to make sure you’re not running a container built for production, not development? |
I did it running
|
Found it! I will try again |
@cuducos @lipemorais Yep! For me it is working now and look good :) I guess it is ok to merge, can I @cuducos ? Is there something more that you think about doing? |
Hey @anaschwendler did tried to change elm code and see it working? |
@lipemorais for it is working fine with elm |
@lipemorais you might try
@anaschwendler if it's working (@lipemorais says it isn't) I have nothing else to add. But in my opening post I described two specific ways to manually tests the development environment and four other specific ways to manually test the production server. From your what I did to test this PR post I've got impression you haven't covered them all. So even if I tested it locally before opening the PR I'm not confident in giving green light if you have not tested those specific points. Specially when @lipemorais tried to and failed. So there might be more work to do, but I do need your help to find out what I got wrong. |
This approach do not worked for me. :/ The point now is that elm container starts to watch butd o not continue alive to keep watching.
It starts and finish for some reason:
|
I'm not sure what you mean there… I don't remember the proper outputs by heart, but Finished 'elm' after 28 s is kind of acceptable if changing a |
It's alive! It worked here and I able to see it working fine. What I mentioned before about finishing log is a normal behaviour as @cuducos just said. |
Last check and I'll merge! |
What is the problem?
Our Docker environment currently is focused on development, so we actually cannot use it to try to enhance our deploy pipeline.
How can this be addressed?
I created a different Docker setups do differ development and production environments:
docker-compose.yml
has stuff common for both environmentsdocker-compose.override.yml
(loaded by default) has stuff inherent to a development environmentdocker-compose.prod.yml
(need to be loaded manually) has stuff inherent to a production environmentDifferences between both environment are documented in the new version of the
README.md
file.Who could help with this issue?
Anyone willing to test it, but specially @Irio @lipemorais @gomex @pedrommone who have helped us a lot with Docker stuff.
Not so obvious things I would test if I were a code reviewer working on the development environment (eg
docker-compose up
and checkinglocalhost:8000
):.elm
file and (after a while, compilation is not instantaneous) see changes refreshing the browser?.py
or.html
file and see changes refreshing the browser?Basic things I would test in both environment (production loads at
localhost:80
):contrib/data/
using paths like/mnt/data/
inside the container?Next steps
docker-compose.yml
to make code review easier (otherwisedocker-compose
would load images from Docker Hub), so if this PR gets approved a new PR is needed to get these images backelm-doc-test
is basically deprecated I removed Elm test suit in 5f47f66 and if this PR goes through I'll open an Issue do we remember to bring it back with the newelm-verify-examples