Skip to content

build: docker setup #23

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

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

build: docker setup #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9a45e40
docs: add cotribution guidelines #19
dedoussis May 23, 2021
268f0d7
Use correct github link references
dedoussis May 23, 2021
2c9cc94
Typo
dedoussis May 23, 2021
6305e8e
Typo no. 2
dedoussis May 23, 2021
1a1bfbc
Add e2e testing suite roadmap item
dedoussis May 23, 2021
6c216ce
build: added docker files to make setup more convenient
pedrohbtp May 24, 2021
e6d954d
docs: added instructions for docker setup
pedrohbtp May 24, 2021
01322bb
build: only map /hooks from container to host
pedrohbtp May 24, 2021
0a45dec
docs: changed precommit to pre-commit
pedrohbtp May 24, 2021
60c42e9
Merge branch 'main' into docker-setup
pedrohbtp May 24, 2021
54c679a
docs: readded bash to pre-commit command
pedrohbtp May 24, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 37 additions & 15 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,21 @@ The current areas of focus are:

## Table of Contents 📖

1. [Roadmap](#roadmap)
1. [Support questions & reporting bugs](#support-questions--reporting-bugs)
1. [Development](#development)
1. [Project structure](#project-structure)
1. [Environment setup](#environment-setup)
1. [Coding style](#coding-style)
1. [Checks & Testing](#checks--testing)
1. [Docs](#docs)
1. [Release](#release)
1. [Finally](#finally)

@dedoussis dedoussis May 26, 2021

Copy link
Copy Markdown
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.

- [Contributing](#contributing)
- [Table of Contents 📖](#table-of-contents-)
- [Roadmap](#roadmap)
- [Support questions & reporting bugs](#support-questions--reporting-bugs)
- [Development](#development)
- [Project structure](#project-structure)
- [Environment setup using docker](#environment-setup-using-docker)
- [Environment setup without docker](#environment-setup-without-docker)
- [Coding style](#coding-style)
- [Checks & Testing](#checks--testing)
- [Docs](#docs)
- [Release](#release)
- [Finally](#finally)


## Roadmap

Expand Down Expand Up @@ -64,7 +69,22 @@ root/
* `tests` is the source of the entire test suite, consisting of unit and integration tests.
* The top level `Makefile` is a toolbox of useful commands for installing dependencies as well as testing, linting and packaging the code. It is also used as the entrypoint interface for all CI/CD operations.

### Environment setup


### Environment setup using docker

For convenience, you can setup the development environment using the Dockerfile and docker-compose files provided in the repo.

To set it up, run the command:

``` bash
docker-compose run asynction

@dedoussis dedoussis May 26, 2021

Copy link
Copy Markdown
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.

@dedoussis dedoussis May 26, 2021

Copy link
Copy Markdown
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.

@dedoussis dedoussis May 26, 2021

Copy link
Copy Markdown
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.

@pedrohbtp pedrohbtp May 26, 2021

Copy link
Copy Markdown
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

@dedoussis dedoussis May 26, 2021 •
edited
Loading

Copy link
Copy Markdown
Owner

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. 👍

```

It will install everything and start the container with bash attached.

### Environment setup without docker


Use python3.7 or higher.

Expand Down Expand Up @@ -92,17 +112,19 @@ Use python3.7 or higher.

1. Install the pre-commit hooks

```bash
$ precommit install
```
``` bash

@dedoussis dedoussis May 26, 2021

Copy link
Copy Markdown
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
```

@dedoussis dedoussis May 26, 2021

Copy link
Copy Markdown
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 🙈

$ pre-commit install
```

### 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!__

@dedoussis dedoussis May 26, 2021

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

🚀



### Checks & Testing

Expand Down
8 changes: 8 additions & 0 deletions Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
FROM python:3.7
WORKDIR /usr/app
ADD . /usr/app
RUN python -m pip install --upgrade pip setuptools
RUN make all-install
RUN pre-commit install
# initializes all pre-commit envs
RUN pre-commit
13 changes: 13 additions & 0 deletions docker-compose.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
version: "3.9"

services:
asynction:
image: asynction
volumes:
- .:/usr/app
# use the .git from the container because it
# has pre-commit setup
- /usr/app/.git/hooks
build: .
entrypoint: bash