-
Notifications
You must be signed in to change notification settings - Fork 11
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
base: master
Are you sure you want to change the base?
Conversation
jinja2 = ">=2.8" | ||
click = ">=6.6" | ||
terminaltables = ">=3.0.0" |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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.
expose: | ||
- "8011" |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
volumes: | ||
- ./sammy/:/code/sammy |
There was a problem hiding this comment.
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
volumes: | |
- ./sammy/:/code/sammy | |
volumes: | |
- .:/code/ |
Changes
master