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

Feature: Prototype Ergo-base dockerfile #99

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

Conversation

herrmt
Copy link

@herrmt herrmt commented Aug 12, 2022

Prototype docker file for creating proposed ergo-base image for ergo components.

Comment on lines +4 to +15
if [ -f "requirements.txt" ]; then
echo "installing via requirements.txt"
# if using requirements.txt, might be recommended to use ssh installation
python3 -m pip install -r requirements.txt
elif [ -f "setup.py" ]; then
echo "installing via setup.py"
pip install .
pip install -e ../../lib
elif [ -f "Pipfile" ]; then
echo "installing via pipenv"
export PYPI_PASSWORD=$(cat /run/secrets/PYPI_PASSWORD)
pipenv install
Copy link
Contributor

Choose a reason for hiding this comment

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

Could probably scrap these

Copy link
Contributor

Choose a reason for hiding this comment

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

i agree! our logic is much simpler if we remove installing via requirements.txt, via setup.py, etc and just consolidate on peotry. Thinking of opening up a similar PR for ergo-infra


if [ ! -f ${NAMESPACES}/${NAMESPACE} ]
then
echo "Namespace ${NAMESPACES}/${NAMESPACE} does not exist - dynamically generating..."
Copy link
Contributor

Choose a reason for hiding this comment

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

This is v nice! When do we need these dynamic configs?

Copy link
Contributor

Choose a reason for hiding this comment

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

oooo do like echos for more usability!

any reason for not updating the error messages in the ergo codebase itself to be more descriptive / helpful?

RUN mkdir ${BASE}/namespaces

# hardcoded for root user
ENV PATH="/root/.local/bin:${PATH}:/root/.poetry/bin"
Copy link
Contributor

Choose a reason for hiding this comment

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

the .poetry directory won't exist without a valid poetry installation step

# syntax = docker/dockerfile:1.0-experimental
FROM python:3.9-slim as ergo_base
ARG comp
ARG qed_dir="."
Copy link
Contributor

Choose a reason for hiding this comment

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

this might be a little overly specific to qed

@@ -0,0 +1,40 @@
all: clean build
synch:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit sorry

Suggested change
synch:
sync:

elif [ -f "pyproject.toml" ]; then
echo "installing via poetry"
export POETRY_HTTP_BASIC_NAUTILUS_USERNAME=$(cat /run/secrets/POETRY_HTTP_BASIC_NAUTILUS_USERNAME)
export POETRY_HTTP_BASIC_NAUTILUS_PASSWORD=$(cat /run/secrets/POETRY_HTTP_BASIC_NAUTILUS_PASSWORD)
Copy link
Contributor

Choose a reason for hiding this comment

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

there's no need to keep both secrets

Suggested change
export POETRY_HTTP_BASIC_NAUTILUS_PASSWORD=$(cat /run/secrets/POETRY_HTTP_BASIC_NAUTILUS_PASSWORD)
export POETRY_HTTP_BASIC_NAUTILUS_PASSWORD=$(cat /run/secrets/PYPI_PASSWORD)

pipenv install
elif [ -f "pyproject.toml" ]; then
echo "installing via poetry"
export POETRY_HTTP_BASIC_NAUTILUS_USERNAME=$(cat /run/secrets/POETRY_HTTP_BASIC_NAUTILUS_USERNAME)
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't it enough for the password to be secret?

Suggested change
export POETRY_HTTP_BASIC_NAUTILUS_USERNAME=$(cat /run/secrets/POETRY_HTTP_BASIC_NAUTILUS_USERNAME)
export POETRY_HTTP_BASIC_NAUTILUS_USERNAME=nautiluslabs

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.

4 participants