-
Notifications
You must be signed in to change notification settings - Fork 0
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
Support Poetry Config #3
Conversation
@@ -2,16 +2,19 @@ | |||
|
|||
set -o pipefail | |||
|
|||
python3 -m venv dbt |
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.
Removed creating and entering the virtual env which didn't seem necessary. For the poetry setup, those dependencies will also be installed outside of a virtual environment so the base dbt
functions will behave the same.
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.
I think that I added this initially as I was seeing some warnings suggesting a venv
python3 -m venv dbt | ||
source dbt/bin/activate | ||
|
||
# Install python dependencies | ||
if [ -z ${DBT_MZ_VERSION} ] |
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.
Don't love this order of check if DBT_MZ_VERSION
is set, then check POETRY_VERSION
and then finally use latest DBT_MZ_VERSION
.
@@ -2,16 +2,19 @@ | |||
|
|||
set -o pipefail | |||
|
|||
python3 -m venv dbt |
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.
I think that I added this initially as I was seeing some warnings suggesting a venv
Dockerfile
Outdated
# Copy poetry files into the container | ||
COPY poetry.lock pyproject.toml ./ |
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.
Shouldn't this be conditional on whether POETRY_VERSION
is set? It also looks like pyproject.toml
and poetry.lock
shouldn't be committed to the repo outside of /tests
.
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.
@bobbyiliev not sure on the best location for these files. I was treating these like the profiles.yml.template
as an example of what it might look like in the repo being used. Though true the COPY should probably be conditional.
README.md
Outdated
@@ -20,6 +20,7 @@ You can override the default values for the following variables: | |||
| Name | Description | Default | | |||
| -------------------- | -------------------------------------------- | ------------- | | |||
| `DBT_MZ_VERSION` | `dbt-materialize` adapter version to install | `latest` | | |||
| `POETRY_VERSION` | Poetry version if dbt project is managed with poetry. Cannot be used with `DBT_MZ_VERSION` | | |
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.
| `POETRY_VERSION` | Poetry version if dbt project is managed with poetry. Cannot be used with `DBT_MZ_VERSION` | | | |
| `POETRY_VERSION` | Poetry version, if the dbt project is managed with [Poetry](https://python-poetry.org/). Exclusive with `DBT_MZ_VERSION`. | | |
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.
Wouldn't the Poetry version also be available in the user's pyproject.toml
? Sounds a bit like a chicken and egg problem. I'm wondering if we could go from that file and pip install .
instead of requiring users to set the Poetry version on the side. IIUC, this would not install dev dependencies by default (see Poetry #3514), but dependency management seems to have changed quite a bit in 1.2.0.
Support setting the dbt version from the poetry configuration if set. Decided to just install their poetry config via the Github action rather than just parsing out the dbt version. Using the
poetry.lock
seems a little messy and while poetry does support getting information about individual packages, you would still need to parse the output. So something likepoetry show dbt-snowflake
would produce: