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

[CT-748] feat: resolve packages with same git repo and unique subdirectory #8322

Conversation

philippeboyd
Copy link
Contributor

resolves #5374

Problem

It is currently impossible to refer multiple git packages that are contained within the same mono repo such as:

  - git: "git@github.com:acme/git-repo.git"
    subdirectory: "dbt-source-pkg-1"
    revision: "dbt-source-pkg-1-v0.1.12"
  - git: "git@github.com:acme/git-repo.git" # This will not work...
    subdirectory: "dbt-source-pkg-2" # This will not work...
    revision: "dbt-source-pkg-2-v0.1.2" # This will not work...

Solution

Use the git and subdirectory values as the unique combination of values when managing dependencies. This will enable dbt to clone multiple git repositories but with different subdirectories; treating them as differents packages.

packages:
  - package: "calogica/dbt_date"
    version: [ ">=0.5.4", "<0.8.0" ]
  - package: "dbt-labs/dbt_utils"
    version: [ ">=1.0.0", "<2.0.0" ]
  - git: "git@github.com:acme/git-repo.git"
    subdirectory: "dbt-source-pkg-1"
    revision: "dbt-source-pkg-1-v0.1.12"
  - git: "git@github.com:acme/git-repo.git"
    subdirectory: "dbt-source-pkg-2"
    revision: "dbt-source-pkg-2-v0.1.2"

The following will import correctly

❯ dbt deps
14:49:31  Running with dbt=1.5.1
14:49:41  Installing calogica/dbt_date
14:49:41  Installed from version 0.7.2
14:49:41  Up to date!
14:49:41  Installing dbt-labs/dbt_utils
14:49:42  Installed from version 1.1.1
14:49:42  Up to date!
14:49:42  Installing git@github.com:acme/git-repo.git/dbt-source-pkg-1
14:49:43  Installed from revision dbt-source-pkg-1-v0.1.12
14:49:43  and subdirectory dbt-source-pkg-1
14:49:43  Installing git@github.com:acme/git-repo.git/dbt-source-pkg-2
14:49:44  Installed from revision dbt-source-pkg-2-v0.1.2
14:49:44  and subdirectory dbt-source-pkg-2

Checklist

  • I have read the contributing guide and understand what's expected of me
  • 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
  • This PR has no interface changes (e.g. macros, cli, logs, json artifacts, config files, adapter interface, etc) or this PR has already received feedback and approval from Product or DX

@philippeboyd philippeboyd requested a review from a team as a code owner August 4, 2023 14:46
@cla-bot
Copy link

cla-bot bot commented Aug 4, 2023

Thanks for your pull request, and welcome to our community! We require contributors to sign our Contributor License Agreement and we don't seem to have your signature on file. Check out this article for more information on why we have a CLA.

In order for us to review and merge your code, please submit the Individual Contributor License Agreement form attached above above. If you have questions about the CLA, or if you believe you've received this message in error, please reach out through a comment on this PR.

CLA has not been signed by users: @philippeboyd

@philippeboyd
Copy link
Contributor Author

CLA has just been signed.

@philippeboyd philippeboyd force-pushed the feature/ct-748-resolve-multiple-git-repos branch from fd2a4bc to f9392f1 Compare August 4, 2023 15:05
@cla-bot cla-bot bot added the cla:yes label Aug 4, 2023
@philippeboyd philippeboyd changed the title feat: resolve multiple packages with same git repo and unique subdirectory [CT-748] feat: resolve multiple packages with same git repo and unique subdirectory Aug 4, 2023
@philippeboyd philippeboyd changed the title [CT-748] feat: resolve multiple packages with same git repo and unique subdirectory [CT-748] feat: resolve packages with same git repo and unique subdirectory Aug 4, 2023
@philippeboyd
Copy link
Contributor Author

@jtcohen6 @leahwicz let me know if this PR makes sense to handle multiple same git repo and unique subdirectory

we're already using this solution (forked dbt core) at our company and it's working really well.

@philippeboyd
Copy link
Contributor Author

@jtcohen6 @leahwicz, is there anything missing for this PR to be reviewed/merged?

@jaklan
Copy link

jaklan commented Sep 1, 2023

@jtcohen6 @leahwicz Also bumping that MR - it's absolutely crucial for monorepo users and we are waiting for the fix for more than year already.

@graciegoheen graciegoheen added the ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering label Sep 14, 2023
@github-actions
Copy link
Contributor

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

@philippeboyd philippeboyd force-pushed the feature/ct-748-resolve-multiple-git-repos branch from f9392f1 to 0648827 Compare September 16, 2023 16:06
@philippeboyd
Copy link
Contributor Author

changelog entry just added

@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

Patch coverage: 83.33% and project coverage change: -0.06% ⚠️

Comparison is base (3f5ebe8) 86.60% compared to head (0648827) 86.55%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8322      +/-   ##
==========================================
- Coverage   86.60%   86.55%   -0.06%     
==========================================
  Files         175      175              
  Lines       25638    25643       +5     
==========================================
- Hits        22204    22195       -9     
- Misses       3434     3448      +14     
Flag Coverage Δ
integration 83.34% <75.00%> (-0.13%) ⬇️
unit 65.09% <58.33%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
core/dbt/deps/git.py 93.25% <83.33%> (-1.98%) ⬇️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@graciegoheen
Copy link
Contributor

Thanks @philippeboyd!

One case I wanted to check was whether or not this PR changes the package name, for the purposes of something like setting the dispatch config.

Because the package name is coming from the dbt_project.yml in the package, this shouldn't break the package naming convention!

Queuing this up for review with one of our engineers :)

Copy link
Contributor

@peterallenwebb peterallenwebb left a comment

Choose a reason for hiding this comment

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

@peterallenwebb @graciegoheen and @dbeatty10 think this looks good!

@peterallenwebb peterallenwebb merged commit f65e4b6 into dbt-labs:main Sep 27, 2023
49 checks passed
@philippeboyd philippeboyd deleted the feature/ct-748-resolve-multiple-git-repos branch October 2, 2023 14:08
@philippeboyd philippeboyd restored the feature/ct-748-resolve-multiple-git-repos branch October 18, 2023 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes ready_for_review Externally contributed PR has functional approval, ready for code review from Core engineering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-748] [Enhancement] dbt deps should resolve multiple packages with same git: repo and unique subdirectory:
7 participants