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

Support Poetry Config #3

Closed
wants to merge 3 commits into from
Closed

Support Poetry Config #3

wants to merge 3 commits into from

Conversation

dehume
Copy link
Contributor

@dehume dehume commented Feb 17, 2023

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 like poetry show dbt-snowflake would produce:

 name         : dbt-snowflake
 version      : 1.1.0
 description  : The Snowflake adapter plugin for dbt

dependencies
 - cryptography >=3.2,<4
 - dbt-core >=1.1.0,<1.2.0
 - requests <3.0.0
 - snowflake-connector-python >=2.4.1,<2.8.0

@dehume dehume linked an issue Feb 17, 2023 that may be closed by this pull request
@@ -2,16 +2,19 @@

set -o pipefail

python3 -m venv dbt
Copy link
Contributor Author

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.

Copy link
Contributor

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} ]
Copy link
Contributor Author

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
Copy link
Contributor

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

.github/workflows/tests.yaml Outdated Show resolved Hide resolved
.github/workflows/tests.yaml Outdated Show resolved Hide resolved
Dockerfile Outdated
Comment on lines 20 to 21
# Copy poetry files into the container
COPY poetry.lock pyproject.toml ./

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.

Copy link
Contributor Author

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` | |

Choose a reason for hiding this comment

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

Suggested change
| `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`. | |

Copy link

@morsapaes morsapaes Feb 20, 2023

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.

@dehume dehume closed this Feb 27, 2023
@dehume dehume deleted the Support-Poetry-Config branch February 27, 2023 20:45
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.

Support pulling dbt version from dependency management config files
3 participants