-
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
Avoid recursing when installing a project to a subproject via copytree #9734
base: main
Are you sure you want to change the base?
Conversation
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: @djbelknap |
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. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9734 +/- ##
==========================================
+ Coverage 88.82% 88.84% +0.01%
==========================================
Files 180 180
Lines 22584 22584
==========================================
+ Hits 20060 20064 +4
+ Misses 2524 2520 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
shutil.copytree( | ||
src_path, | ||
dest_path, | ||
ignore=lambda directory, contents: project.packages_install_path | ||
if directory == project.project_root | ||
else [], | ||
) |
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.
What
"Don't install yourself into your own dbt_packages
folder."
How
shutil.copytree
recursively copies an entire directory tree rooted at src
to a directory named dst
.
The new ignore
parameter introduces a base case under which this recursion should stop. It essentially says:
"No dbt project ever needs to install itself within its own packages_install_path
folder 😄. So when the current directory being processed is the same as this dbt project, then do not recursively copy the configured packages_install_path
directory because it's already covered. Just ignore it!"
Why
The ignore
parameter is being added here to prevent a dbt project from installing itself via dbt deps
. As mentioned in #9719, this can happen if:
- a dbt project is a subfolder of a
local
dependency listed withinpackages.yml
, and - the host operating system doesn't support symlinks.
The 1st item is common for CI testing for packages like dbt_utils
, and the 2nd is most common in legacy versions of Windows.
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.
Windows 10 users can possibly avoid this code path by turning on Developer Mode (which may allow creating Symbolic Links without requiring admin privileges) [1].
But this PR is specifically intended to cover the case where the user's system doesn't allow symlinks for whatever reason. So Developer Mode for Windows 10 is not a sufficient workaround for all cases.
resolves #9719
Problem
When installing a local package that contains the current project as a subfolder (like the
integration_tests
project in dbt-utils), if the symlink fails the copytree recursively copies the current project repeatedly until the path length limit is reached.Solution
Use the
ignore
parameter of copytree to ignore the current project's packages directory to avoid repeated recursion.Checklist