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.

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

Docker dev env #498

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

Conversation

patroqueeet
Copy link

Description

simple docker compose setup to run tests

Motivation and Context

easier dev start

How Has This Been Tested?

by using it :)

Screenshots (if appropriate):

Types of changes

config files

Checklist:

  • My code follows the code style of this project.
  • [ x My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@dopry
Copy link
Contributor

dopry commented May 12, 2022

@patroqueeet so this simplifies dev startup by not requiring the developer have python installed locally?

@patroqueeet
Copy link
Author

@patroqueeet so this simplifies dev startup by not requiring the developer have python installed locally?

I have a Mac and at least to me, it helped prevent any kind of local setup. check the code out, run the tests via docker and be happy :)

up to you if you consider it helpful. was at least to me

@dopry
Copy link
Contributor

dopry commented May 12, 2022

I do consider it helpful. I work on windows, and supporting multiple versions of python on windows is not always easy. The venv code makes a lot of assumptions about Windows that aren't necessarily true.

I think this would make a good starting point to improving the experience for developers who want to contribute. Can you add instructions for running the example project with docker compose as well? I think it's important to facilistate E2E testing along with unit testing.

If you have the capacity I think there are some improvements to this concept that would further improve the DX and streamline the experience for contributors, but might be better to consider as follow on work.

  1. User a pre-existing image rather than building one for development. It would be nice to avoid the build step if possible, just because caching can be an issue.
  2. compatibility with Docker Desktop's Development Environments

@patroqueeet
Copy link
Author

@dopry this has been added now. NOTE: the example app is broken with Djv4. You'd need to add "Django>=2.2,<4", to your setup.py :)

Copy link
Collaborator

@moggers87 moggers87 left a comment

Choose a reason for hiding this comment

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

Just a few comments. I'm not sure what this adds to the project and I'm skeptical that this will be well maintained in the future (I don't think any of the current devs use Docker regularly, but maybe I'm wrong?)

@@ -32,6 +39,10 @@ covering all supported Python and Django version with::

tox

With docker run tests::

docker-compose run --rm cli bash -c "pip install -r requirements_dev.txt; make test"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this really easier than running pip install --user tox; tox? If Docker supports one-shot services maybe we should have one to run the tests?

Copy link
Author

Choose a reason for hiding this comment

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

that is always a matter of personal flavour. I like to prevent messing around locally with dozens of different virtualenvs. but I'm not the maintainer. I offered what I ran locally and others asked for it. so its up to you to find the right way for this lib.

@@ -0,0 +1,16 @@
version: "3.9"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this the Python version to be used? If not, where is that defined?

Copy link
Author

Choose a reason for hiding this comment

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

rtfm, sry.

@dopry
Copy link
Contributor

dopry commented May 26, 2022

@dopry this has been added now. NOTE: the example app is broken with Djv4. You'd need to add "Django>=2.2,<4", to your setup.py :)

I'll take a look later today. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants