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

build: docker setup #23

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

pedrohbtp
Copy link

My changes in this PR:

  • I added a Dockerfile and a docker-compose to make it more convenient for people that want to contribute and use docker to set things up
  • Updated CONTRIBUTING.md showing how to run using docker
  • Changed the typo precommit to pre-commit that slipped through the review you previously asked

Copy link
Owner

@dedoussis dedoussis left a comment

Choose a reason for hiding this comment

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

Thanks for putting together a PR! Really appreciated 💯

Kudos for spotting the pre-commit typo's.

I generally like the docker instructions idea, but I think having the Dockerfile and docker-compose.yml files at the root of the repo takes it a bit too far.

```bash
$ precommit install
```
``` bash
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
``` bash
```bash
```

Copy link
Owner

Choose a reason for hiding this comment

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

Just some indentation. Not important, but my local markdown linter complains 🙈


### Coding style

* All Python code must follow the [PEP 8](https://www.python.org/dev/peps/pep-0008/) guidelines.
* Type annotations are mandatory (even in tests). Avoid the use of `typing.Any` and `# type: ignore`.
* All parts of the public API (exposed via `asynction.__init__.py`) should be documented with docstrings.

__Make sure you run `precommit` before every commit!__

__Make sure you run `pre-commit` before every commit!__
Copy link
Owner

Choose a reason for hiding this comment

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

🚀

To set it up, run the command:

``` bash
docker-compose run asynction
Copy link
Owner

Choose a reason for hiding this comment

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

Although I really like having docker as my local dev environment, this is not the case for everyone. I agree that it would be beneficial to have a docker setup section in the contributing guidelines, however I don't think we should have a Dockerfile and docker-compose featuring at the root of the repo. If we were going down this path, then we could also include vscode or PyCharm configurations (since a lot of people use their IDE to spin up the environment) and we would end up overloading the repository with multiple configuration setups.

Copy link
Owner

Choose a reason for hiding this comment

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

Have also taken a look at other repos of popular python web frameworks such as Flask, django and aiohttp.

None of these include docker setups, so I would prefer to adopt the same philosophy for Asynction.

Copy link
Owner

Choose a reason for hiding this comment

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

However, I do like your idea of having some quick docker setup instructions in the contributing guidelines. We could do this with a docker run one liner. For example:

docker run -it --rm \
  -v ${PWD}:/opt/workspaces/asynction \
  -w /opt/workspaces/asynction \
  python:3.7 bash -c "python -m pip install --upgrade pip setuptools && make all-install && pre-commit install && exec bash"

I also think that the non-docker setup instructions should be placed above the docker instructions, since most people are likely going to use the non-docker ones.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the reply! I see the point.
Since we are not adding the docker files, how do you want to move forward with this PR?
We could close this one and create a cleaner one with only that docker command and the precommit typo fixed.
Next time I might create an issue first to avoid these problems

Copy link
Owner

@dedoussis dedoussis May 26, 2021

Choose a reason for hiding this comment

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

We could close this one and create a cleaner one with only that docker command and the precommit typo fixed.

Up to you -- whatever works for you best. 👍

1. [Docs](#docs)
1. [Release](#release)
1. [Finally](#finally)

Copy link
Owner

Choose a reason for hiding this comment

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

I prefer it the way it was. More concise. No one is going to look for the table of contents while reading the table of contents.

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.

2 participants