-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move to a microservice architecture #125
Conversation
Lots of awesome work here @ifed3. The infrastructure work looks great. The cleaner directory structure and separate Dockerfiles make it much easier to understand the codebase and to isolate changes related to different microservices. However, there are changes that are out of scope of the issue, and this makes this PR quite large and more difficult to review. I would suggest retaining the work related to setting up microservices in this PR and separating out the other changes into different issues/PRs. Some examples of changes I would suggest relocating:
Since I'm asking for a major PR change, I'll hold off on specific review of the infrastructure changes, but I'm looking forward to it - seems like a great way to move forward. |
I agree with @nbilenko here; we really want to make sure our issues contain discrete implementations so work can be tracked more easily and discussions can remain manageable. The issue breakdown provided feels appropriate to me, too. Thanks, |
Good catch @nbilenko with the dataviz service running in production. I've updated that. I think it would be misguided to say that changes are out of scope of this refactor. They are interwoven in a manner that would require more work to separate them out in a new PR. My aim was to build an architecture that can be run locally and on production without major configuration, so with microservices, three high level changes happened:
Switch from pipenv to pip-toolspipenv by default provides a virtual env, serves as both a dependency manager and installer. This is ideal if you want to run a python app on your host machine but is problematic for docker usage. pipenv uses different install commands based on the env: Once there is no need to run the infra on your host machine, it actually becomes both unnecessary and more cumbersome to maintain support for pipenv. Makefile docker-compose aliasesOnce the django app is moved to the api directory, it takes the To keep the aliased docker commands would require creating a new Makefile within the root directory. The aliases were ideal when you could choose an env to run (local vs. production) but became redundant when you're limited to only run the local env So you go from: make %.up: ## Start the (local|production) Docker containers
docker-compose -f docker-compose.yml -f docker-compose.$*.yml up -d to: make local.up ## Start the local Docker containers
docker-compose -f docker-compose.yml -f docker-compose.local.yml up -d However by default, docker-compose searches for two config files ( make local.up
docker-compose -f docker-compose.yml -f docker-compose.override.yml up -d
docker-compose up -d Thus all the aliases essentially became plain and easy to use docker-compose commands and getting familiar with docker-compose syntax will be far more important for one's coding chops than using arbitrary aliases especially for such basic commands. So the aliases became counterproductive both from a code functionality and developer usage standpoint with a microservices architecture README/contributing.mdThese changes relate to the onboarding and use of the micoservices architecture. If I had left them as they were, half of it would be irrelevant and non-functional since it assumes just a django app rather than multiple apps. HoloviewsThis is the change I agree is extraneous and probably out of scope. To your point, @nbilenko, Holoviews was built specifically to solve the problem of displaying pre-made visualizations. Read more here. It can output static displays in many formats such as
Hope that explains some of the changes. I think this conversation would be best be expounded upon and resolved in person. See you all tonight! |
The interweaving of the changes is exactly what my feedback is about - the interweaving complicates both the review and keeping track the history of these changes. The different changes I mentioned are hard to delineate and to keep track of while reading the code. Also, discussion for all changes has to be maintained in a single comment thread. E.g. a chat about pros and cons of dataviz software sounds great, but an infrastructure PR comment thread is not where decisions should be made about what software the team will be using for data visualization. I realize it would take some work for you to separate out the changes, but to read and understand these interwoven changes would instead place a substantial amount of work on the rest of the team. It would also likely to not lead to effective review - best practices tend to recommend limiting code changes to a few hundred lines because reviewers can't reliably detect problems with longer change lists. Let's discuss more tonight. |
b1fd61e
to
66cfdb9
Compare
hey all, This is the new infra document Here's a more thorough breakdown of the changes that happened within this PR. If you have any questions or any confusion, please let me know. Docker
CI (Travis)
Wiki
Python
|
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.
Thank you for the detailed breakdown of the changes and documentation, Ife! I added inline comments about changes I had to make to get things running. Overall, looks great to me.
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.
Very cool @ifed3 ! Thanks for this. I learned a lot from reviewing this -- extending docker-compose files, dockerize, pip-tools.
I did my best to understand the web service, SSL, letsencrypt, etc., but can't claim to be very knowledgeable about that, so I'll trust that you made it work. At the very least, I was able to get it up and running using the nice docs you provided.
I second @nbilenko 's point about preferring smaller PRs, so let's keep that in mind in the future. Approved!
name: Upgrade WOAQ infrastructure
about: #122
title: Set up microservices
labels: infrastructure, api, web, dataviz
reviewers: @r-b-g-b, @theecrit, @nbilenko, @TangoYankee
Checklist
make local.shell
make quality
make local.shell
make test
Description
Issue: #122
Thanks for your patience. This PR is finally ready.
and multiple services instead of a single one
docker-compose up -d --build
instead ofmake local.build
+make local.up
to build and run all servicesdocker-compose up -d --build service_name
to run a specific servicedocker-compose down
instead ofmake local.down
docker-compose run service_name /bin/bash
instead ofmake local.shell
to enter the shell of a specific servicecd
into the correct directory if you want to run commands for a specific servicep.s there are still some outstanding issues relating to devops/infrastructure that will need to be completed in the near future: writing tests for the client service, creating a Heroku container configuration file, setting up the Heroku deploy process, pushing container images to Docker hub to enable faster CI builds