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

Issue 275: Support dbt package dependencies in Git subdirectories #3267

Merged

Conversation

dmateusp
Copy link
Contributor

@dmateusp dmateusp commented Apr 15, 2021

resolves #275

Description

  • Add an optional subdirectory key for Git packages.
  • Use sparse-checkout to pull only that directory.
  • Install dbt package from that subdirectory.
  • Make sure the git version of the user is >= 2.25.0, the version that introduces --sparse.

Checklist

  • I have signed the CLA (Or at least I remember signing it a while back)
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@cla-bot cla-bot bot added the cla:yes label Apr 15, 2021
@dmateusp dmateusp marked this pull request as ready for review April 15, 2021 11:14
# 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',
Copy link
Contributor Author

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 :)

@dmateusp dmateusp changed the title Support dbt package dependencies in Git subdirectories Issue 275: Support dbt package dependencies in Git subdirectories Apr 15, 2021
@@ -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 \
Copy link
Contributor Author

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

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

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

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

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

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

This is looking great @dmateusp! Once the TODOs are sorted, I think this will be good to go.

@kwigley Whenever you get a chance, could we publish a new fishtownanalytics/test-container with git>=2.25?

Comment on lines 104 to 110
# 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'
}]
Copy link
Contributor

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:

  1. 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.)

  1. 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'
    }]
  1. We could add a branch to dbt-utils, similar to move_dbt_utils_to_subdir... and hope it's never accidentally deleted.

Copy link
Contributor Author

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 :)

@dmateusp dmateusp force-pushed the dmateusp/275/support_github_subdirs branch from 76b9bf4 to dc3524f Compare April 19, 2021 13:40
@@ -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(
Copy link
Contributor Author

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)

@dmateusp
Copy link
Contributor Author

@kwigley let me know if I can help with the TODOs, I'm on the dbt Slack with the same handle

@kwigley
Copy link
Contributor

kwigley commented Apr 27, 2021

@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 fishtownanalytics/test-container:12 using the Dockerfile from this branch, you should be able to update CI with that new image 👍

Comment on lines 115 to 125
# 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',
}],
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# 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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done! thanks!

@dmateusp dmateusp force-pushed the dmateusp/275/support_github_subdirs branch from 5186407 to e1234e0 Compare April 28, 2021 09:32
Copy link
Contributor

@jtcohen6 jtcohen6 left a 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 :)

@jtcohen6 jtcohen6 merged commit 5fb36e3 into dbt-labs:develop Apr 28, 2021
iknox-fa pushed a commit that referenced this pull request Feb 8, 2022
)

* 🔨 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

support dbt packages that are not located at the root directory of a repo
3 participants