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

Add Dev Container configuration #8024

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

Conversation

savannahostrowski
Copy link
Contributor

@savannahostrowski savannahostrowski commented May 27, 2023

This PR adds a Dev Container configuration to Poetry. By adding this to the repo, contributors will be able to open the Poetry codebase in a Dev Container (via Visual Studio Code or Codespaces, for example) and have the local development environment setup for them per https://python-poetry.org/docs/contributing/.

I've also added a note to the docs to explain how to use the configuration for easy contributing! No tests applicable here.

@@ -0,0 +1,10 @@
ARG IMAGE=bullseye
Copy link
Contributor

Choose a reason for hiding this comment

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

using bulleye is for any particular reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reading over things again, I realized I could simplify some things/ensure it's using the latest version of Python. I'm using the devcontainer images that Microsoft publishes to MCR.

@@ -0,0 +1,4 @@
poetry install
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know the Devcontainer workflow. The poetry run pre-commit install confuse me. I don't know if the purpose of setup.sh is run before start working in the new feature/bug/etc, in that way the pre-commit install make sense for me, but the other commands don't.
Or when I finished to work in my new feature/bug/etc the setup.sh run, in that case I don't know if I want run the pre-commit install.
IMO it would be great write the new workflow (obviously if is necessary) in the documentation.

In any case, I like this idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, the setup.sh script runs just one time after the image has been created + the editor has launched (see postCreateCommand here).

The commands in this script are those which are denoted in https://python-poetry.org/docs/contributing/#local-development. You probably don't need to run poetry run mypy or poetry run pytest but I've included them to make sure everything is working as expected (doesn't really hurt to run them).

This script/configuration doesn't change anything about the core development workflow as prescribed in the docs today.

@savannahostrowski
Copy link
Contributor Author

Just wanted to ping @Secrus on this since I chatted with them on Discord. Any feedback would be appreciated!

.devcontainer/Dockerfile Outdated Show resolved Hide resolved
@neersighted neersighted self-requested a review July 7, 2023 21:52
Comment on lines +3 to +9
RUN export DEBIAN_FRONTEND=noninteractive \
&& apt-get update && apt-get install -y xdg-utils \
&& apt-get clean -y && rm -rf /var/lib/apt/lists/*

ENV POETRY_HOME="/opt/poetry"
ENV PATH="$POETRY_HOME/bin:$PATH"
RUN curl -sSL https://install.python-poetry.org | python3 -
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
RUN export DEBIAN_FRONTEND=noninteractive \
&& apt-get update && apt-get install -y xdg-utils \
&& apt-get clean -y && rm -rf /var/lib/apt/lists/*
ENV POETRY_HOME="/opt/poetry"
ENV PATH="$POETRY_HOME/bin:$PATH"
RUN curl -sSL https://install.python-poetry.org | python3 -
RUN pipx install poetry

Comment on lines +3 to +5
RUN export DEBIAN_FRONTEND=noninteractive \
&& apt-get update && apt-get install -y xdg-utils \
&& apt-get clean -y && rm -rf /var/lib/apt/lists/*
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need xdg-utils? Does this add anything for users of a container used only via TTY?

Copy link
Member

Choose a reason for hiding this comment

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

It looks like this might relate to devcontainers/images#885 (not sure how VSCode wires that up -- maybe a script that emits an escape sequence?)... I'd rather let upstream figure this out (if we keep the Microsoft image) than try and solve it downstream.

@@ -0,0 +1,9 @@
FROM mcr.microsoft.com/devcontainers/python:3-bookworm
Copy link
Member

@neersighted neersighted Feb 25, 2024

Choose a reason for hiding this comment

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

Any chance we can use a standard library/python base image here? If not, what does the Microsoft-supplied base image add that enhances use of Dev Containers?

mcr.microsoft.com tends to be pretty slow for users in different parts of the world, compared to Docker Hub; though this is a secondary concern for me.

Comment on lines +7 to +9
ENV POETRY_HOME="/opt/poetry"
ENV PATH="$POETRY_HOME/bin:$PATH"
RUN curl -sSL https://install.python-poetry.org | python3 -
Copy link
Member

Choose a reason for hiding this comment

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

We don't really want to depend on the script here, and we're not actively encourage it's use either. Instead a manual install flow is perfectly suitable, in my opinion:

Suggested change
ENV POETRY_HOME="/opt/poetry"
ENV PATH="$POETRY_HOME/bin:$PATH"
RUN curl -sSL https://install.python-poetry.org | python3 -
ENV POETRY_HOME=/opt/poetry
RUN python3 -m venv $POETRY_HOME && \
$POETRY_HOME/bin/pip install poetry
ENV VIRTUAL_ENV=/opt/poetry-env
RUN python3 -m venv $VIRTUAL_ENV
ENV PATH="$VIRTUAL_ENV/bin:$POETRY_HOME/bin:$PATH"

This way we have a virtualenv holding Poetry (like the script creates, but explicitly controlled), and a separate virtual env for the project. This is a generic template following our recommendations that can be duplicated by other projects making use of Poetry + Dev Containers as well.

Comment on lines +2 to +4
poetry run pytest
poetry run mypy
poetry run pre-commit install
Copy link
Member

Choose a reason for hiding this comment

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

We should drop these lines; if the project is in a failing state we shouldn't cause issues with Dev Container creation... Imagine if a user modifies some files such that ruff is not happy, and then tries to activate Dev Containers with their editor -- now they have to stash or similar to avoid a failure here (and not all users will understand that).

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.

5 participants