Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[Discussion] Dockerised development flow #23

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

[Discussion] Dockerised development flow #23

wants to merge 3 commits into from

Conversation

garry-jeromson
Copy link
Contributor

@garry-jeromson garry-jeromson commented May 15, 2020 •
edited
Loading

This is far from complete, and is meant to serve as a starting point for discussion.

A Dockerized development setup has the advantage that all the developer needs to have installed is make and Docker - all the Python stuff is containerised, making it more accessible (and hopefully, because of that, more "developable"). This, however, is a departure from the current project setup based on tox.

Tox and Docker are two difference approaches to the same problem (testing with multiple Python versions) - it doesn't really make sense to run tox commands within a Docker container, and it may be tricky to support both development approaches simultaneously.

This commit forms the basis of my suggested initial approach:

  • Development requirements should be specified in a requirements-dev.txt file, to allow easy installation outside of tox
  • Runtime requirements should ideally be specified in requirements.txt, to be available outside of setup.py (assuming tox and setup.py can then reference it, to avoid maintaining duplicate lists of the same dependencies)
  • The Dockerfile for the development image takes the Python version as a build argument, to allow easily adding new versions.

@garry-jeromson garry-jeromson changed the title Initial docker-compose setup (WIP) [Discussion] Dockerised development flow May 16, 2020
brunns
brunns reviewed May 16, 2020
View reviewed changes
setup.py Outdated
@@ -18,6 +18,9 @@
logger.warning("README file not found or unreadable.")
long_description = "See https://github.com/brunns/mbtest/"

with open('requirements.txt') as f:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there some way we can do without this? This is how I used to do it, but there is at least one downstream service which needs setup.py to be self contained, IIRC.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be that we just have to live with having two copies of the requirements list - not really a biggie.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, if tox.ini, setup.py and requirements.txt can live independently of each other, it may simplify supporting both non-Dockerised and Dockerised workflows - the Docker containers would rely on the requirements.txt files, and tox would keep on doing its thing. Do you see any potential issues there?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like something in the build ensuring the two are in sync. If we have that, I can live with the duplication.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, better still, can we build requirements.txt from setup.py, make it a generated artifact?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One potential option: https://github.com/jazzband/pip-tools#example-usage-for-pip-compile

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks worth a try!

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Travis is unhappy. I'm not sure why that's not showing up in the PR - I'll look into that.

@garry-jeromson garry-jeromson force-pushed the dockerize-dev-setup branch 5 times, most recently from 4ba5129 to ac7b232 Compare May 18, 2020 09:48
@garry-jeromson
Copy link
Contributor Author

I'd say we're cooking with gas on this now - all that's left to do is to bring the Docker Makefile to feature parity with the tox one, and update the docs.

@brunns
Copy link
Owner

brunns commented May 18, 2020

Can you add a travis target, so we can see if anything breaks the docker build?

@brunns
Copy link
Owner

brunns commented May 18, 2020

Is the Makefile rename necessary? I'd like to continue to be able to just run make precommit before committing.

@garry-jeromson
Copy link
Contributor Author

Is the Makefile rename necessary? I'd like to continue to be able to just run make precommit before committing.

Have changed it back and added an easy way to switch between the two from the command line.

Can you add a travis target, so we can see if anything breaks the docker build?

I'm a Travis noob - not sure if what I added is in the right place.

@garry-jeromson garry-jeromson force-pushed the dockerize-dev-setup branch 3 times, most recently from 7c6e8a0 to 61b432b Compare May 18, 2020 19:26
brunns
brunns reviewed May 19, 2020
View reviewed changes
Copy link
Owner

@brunns brunns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems to be working for me.

Makefile
@@ -88,6 +88,11 @@ repl: ## Python REPL
outdated: ## List outdated dependancies
tox -e py38 -- pip list --outdated

.PHONY: use-docker
use-docker: ## Use Docker-based local development workflow
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This leaves my git status dirty. I wonder if that's avoidable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative would be to have both tox and Docker targets in the same Makefile, and switching which targets are executed based on a variable. That would still leave the variable as a potential dirty status candidate though, and the Makefile would be really long.

Garry Jeromson added 2 commits May 23, 2020 16:16
Set up Docker development workflow
35eb85a
Add precommit target to Docker Makefile
765028c 
This allows running `make precommit` and coming out the other side with a clean Git status.
Update NodeJS version
5ff1dd6 
Probably need to find a more robust way of installation.
@brunns
Copy link
Owner

brunns commented Jun 1, 2020

How are we going with this? BTW, I came across https://github.com/kudulab/dojo which might be useful...

@brunns brunns self-assigned this Jul 9, 2020
@garry-jeromson
Copy link
Contributor Author

garry-jeromson commented Nov 10, 2020 •
edited
Loading

Coming back to this after a loooong time with fresh eyes; I wonder if it's actually going to be useful to anyone. The tox-based workflow isn't really a hurdle at all if you know what you're doing, and the Docker stuff adds complexity - if it's not gonna be used by other folks, perhaps we should just can it?

@brunns
Copy link
Owner

brunns commented Nov 13, 2020

Perhaps. I won't be using it myself, put perhaps it'll be useful to some folks?

@brunns brunns force-pushed the master branch 4 times, most recently from b570521 to 9db229f Compare January 28, 2021 09:50
@brunns brunns force-pushed the master branch 2 times, most recently from 1d4ecbc to 09eedff Compare June 16, 2021 13:59
@brunns brunns force-pushed the master branch 3 times, most recently from 4af0bc2 to e860499 Compare December 31, 2024 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brunns brunns brunns left review comments

Assignees

@brunns brunns

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@garry-jeromson @brunns