-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Issue 275: Support dbt package dependencies in Git subdirectories #3267
Issue 275: Support dbt package dependencies in Git subdirectories #3267
Conversation
test/rpc/test_deps.py
Outdated
# TODO: Change me to something that fishtown-analytics manages! | ||
# here I just moved the dbt_utils code into a subdirectory | ||
{ | ||
'git': 'https:/dmateusp/dbt-utils.git', |
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 needs to be changed before merging :)
@@ -3,6 +3,9 @@ FROM ubuntu:18.04 | |||
ENV DEBIAN_FRONTEND noninteractive | |||
|
|||
RUN apt-get update \ | |||
&& apt-get install -y --no-install-recommends \ | |||
software-properties-common \ | |||
&& add-apt-repository ppa:git-core/ppa -y \ |
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 had to update the git
version in the test container, it was 2.17, needs to be >= 2.25
.circleci/config.yml
Outdated
- image: fishtownanalytics/test-container:11 | ||
# TODO: Ask for someone at fishtown to publish | ||
# a new version of the test container | ||
- image: dmateusp/dbt-test-container |
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.
Will need to push a new test-container version with the updated git version
.circleci/config.yml
Outdated
- image: fishtownanalytics/test-container:11 | ||
# TODO: Ask for someone at fishtown to publish | ||
# a new version of the test container | ||
- image: dmateusp/dbt-test-container |
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.
Will need to push a new test-container version with the updated git 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.
test/rpc/test_deps.py
Outdated
# TODO: Change me to something that fishtown-analytics manages! | ||
# here I just moved the dbt_utils code into a subdirectory | ||
packages = [{ | ||
'git': 'https:/dmateusp/dbt-utils.git', | ||
'revision': 'dmateusp/0.5.0/move_dbt_utils_to_subdir', | ||
'subdirectory': 'dbt_projects/dbt_utils' | ||
}] |
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.
Nice! Ok, here are some options:
- Use a long-lived subdirectory of
dbt-utils
:
packages = [{
'git': 'https:/dmateusp/dbt-utils.git',
'revision': '0.5.0',
'subdirectory': 'integration_tests'
}]
But because integration_tests
has a local dependency on ../
, we'd need to add a dummy dbt_project.yml
file to the directory right above this one. (Those relative paths become relative to the root project location.)
- Use a less-well established "package" that has a lot of project subdirectories (and which would really benefit from the addition of this feature):
packages = [{
'git': 'https:/fishtown-analytics/dbt-labs-experimental-features',
'subdirectory': 'materialized-views'
}]
- We could add a branch to
dbt-utils
, similar tomove_dbt_utils_to_subdir
... and hope it's never accidentally deleted.
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.
Sorry I didn't get if these options were for me or @kwigley ^^, I'd use dbt-labs-experimental-features
on a specific commit sha, then we wouldn't worry about anything breaking the tests :)
76b9bf4
to
dc3524f
Compare
@@ -72,42 +72,80 @@ def deps_with_packages(packages, bad_packages, project_dir, profiles_dir, schema | |||
querier.is_result(querier.async_wait(tok1)) | |||
|
|||
|
|||
@pytest.mark.parametrize( |
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.
@jtcohen6 I parametrized the test instead of the copy paste fyi
(I did this when I merged the changes from the PR that adds support for commits here)
@kwigley let me know if I can help with the TODOs, I'm on the dbt Slack with the same handle |
thanks for bearing with me! I just pushed a new image tagged |
test/rpc/test_deps.py
Outdated
# TODO: Change me to something that fishtown-analytics manages! | ||
# here I just moved the dbt_utils code into a subdirectory | ||
[{ | ||
'git': 'https:/dmateusp/dbt-utils.git', | ||
'revision': 'dmateusp/0.5.0/move_dbt_utils_to_subdir', | ||
'subdirectory': 'dbt_projects/dbt_utils', | ||
}], | ||
[{ | ||
'git': 'https:/fishtown-analytics/dbt-utils.git', | ||
'revision': '0.5.0', | ||
'subdirectory': 'path/to/nonexistent/dir', | ||
}], | ||
), |
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.
# TODO: Change me to something that fishtown-analytics manages! | |
# here I just moved the dbt_utils code into a subdirectory | |
[{ | |
'git': 'https:/dmateusp/dbt-utils.git', | |
'revision': 'dmateusp/0.5.0/move_dbt_utils_to_subdir', | |
'subdirectory': 'dbt_projects/dbt_utils', | |
}], | |
[{ | |
'git': 'https:/fishtown-analytics/dbt-utils.git', | |
'revision': '0.5.0', | |
'subdirectory': 'path/to/nonexistent/dir', | |
}], | |
), | |
[{ | |
'git': 'https:/fishtown-analytics/dbt-labs-experimental-features.git', | |
'revision': '0.0.1', | |
'subdirectory': 'materialized-views', | |
}], | |
[{ | |
'git': 'https:/fishtown-analytics/dbt-labs-experimental-features.git', | |
'revision': '0.0.1', | |
'subdirectory': 'path/to/nonexistent/dir', | |
}], | |
), |
this should do the trick!
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.
Done! thanks!
5186407
to
e1234e0
Compare
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.
@dmateusp This is awesome! Thank you so much for the contribution, and for the wonderful commit messages along the way :)
) * 🔨 Extend git package contract and signatures to pass `subdirectory` * Add sparse checkout logic * ✅ Add test * 🧹 Lint * ✏️ Update CHANGELOG * 🐛 Make os.path.join safe * Use a test-container with an updated `git` version * 🔨 Fix integration tests * 📖 Update CHANGELOG contributors to include this PR * 🧪 Parameterize the test * Use new test-container published by @kwigley (contains more recent version of git) * Use repositories managed by fishtown * 🧘♂️ Merge the CHANGELOG * 🤦♂️ Remove repetition of my contribution on the CHANGELOG Co-authored-by: Jeremy Cohen <[email protected]> automatic commit by git-black, original commits: 5fb36e3
resolves #275
Description
subdirectory
key for Git packages.sparse-checkout
to pull only that directory.subdirectory
.>= 2.25.0
, the version that introduces--sparse
.Checklist
CHANGELOG.md
and added information about my change to the "dbt next" section.