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

Added github workflow. Dockerized and poetrized app. #23

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

Conversation

Zuiluj
Copy link

@Zuiluj Zuiluj commented Sep 7, 2022

Changes

  • Added github actions when pushed to master
  • Dockerized app and enabled jupyter notebook development
  • Added poetry as dependency manager

@Zuiluj Zuiluj requested a review from bjinwright September 7, 2022 09:16
@Zuiluj Zuiluj self-assigned this Sep 7, 2022
@Zuiluj Zuiluj temporarily deployed to Master September 7, 2022 09:16 Inactive
@Zuiluj Zuiluj temporarily deployed to Master September 9, 2022 01:33 Inactive
Comment on lines +10 to +12
jinja2 = ">=2.8"
click = ">=6.6"
terminaltables = ">=3.0.0"

Choose a reason for hiding this comment

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

Given these are only for the cli extras_require in the setup.py, these should be similarly specified in Poetry using an optional group: https://python-poetry.org/docs/managing-dependencies/#optional-groups so that folks installing without need for the CLI functionality can ignore them and install only the library.

readme = "README.md"

[tool.poetry.dependencies]
python = "^3.9"

Choose a reason for hiding this comment

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

This specification resolves to >=3.9.0, <4.0.0. I'd imagine that can be loosened up a bit; the effective minimum version of python for this library definitely isn't 3.9.

Comment on lines +9 to +10
expose:
- "8011"

Choose a reason for hiding this comment

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

expose exposes this port to other services in this Docker network (eg, defined in this docker-compose.yml file). Since you have none, and so no other containers that will want to talk to this one, this isn't necessary.

@@ -0,0 +1,5 @@
FROM capless/capless-docker:jupyter
COPY . /code
RUN python -m pip install --upgrade poetry

Choose a reason for hiding this comment

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

FYI this is not Poetry's recommended method of installing Poetry: https://python-poetry.org/docs/#installation

FROM capless/capless-docker:jupyter
COPY . /code
RUN python -m pip install --upgrade poetry
RUN poetry run pip install --upgrade pip

Choose a reason for hiding this comment

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

Modern versions of Poetry use an internal installer and not pip anymore, so this may not be necessary.

@@ -0,0 +1,19 @@
[tool.poetry]
name = "envs"
version = "0.1.0"

Choose a reason for hiding this comment

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

This should follow the version of the library in the setup.py, and be updated for new releases. setup.py is currently at 1.3, though current version in PyPI is 1.4. This is why using VCS tags and associated tooling is helpful a la #21

@@ -0,0 +1,5 @@
FROM capless/capless-docker:jupyter
COPY . /code

Choose a reason for hiding this comment

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

Having this COPY before the Poetry install will cause Docker's layer caching to assume it needs to reinstall poetry whenever the source code changes. Moving this after installing Poetry will allow that layer to be cached and save time on subsequent builds.

Comment on lines +13 to +14
volumes:
- ./sammy/:/code/sammy

Choose a reason for hiding this comment

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

This may have been copy-pasta'ed from whatever example you used, but wouldn't work based on directories in this repo. Looking at the Dockerfile I"m imagining what you want is

Suggested change
volumes:
- ./sammy/:/code/sammy
volumes:
- .:/code/

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