-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Make deps respect --project-dir #2534
Conversation
4357afe
to
83a4562
Compare
I do not understand how this change causes that test to fail in that way. This test passes locally (both in isolation and in parallel) but consistently fails the exact same way in circle. Nothing about this PR should change that test's behavior and it makes no sense to me that the schemas could already exist. |
Make the unique ID a bit more collision-resistant to try to avoid collisions
9114285
to
e00d1ed
Compare
I made a bunch of silly test changes to drop more schemas and try to avoid collisions, which seems to have fixed the issue |
I'm going to take some more time to comb through the diff in this PR, but I did want to call something out I noticed while debugging: I think we've sort of flip-flopped on regressions for this feature over the last couple of versions of dbt:
Each of the invocations marked
So, the key takeaway is that I'm seeing I made a loom repro'ing this error in this branch: https://www.loom.com/share/54077f3fc52344d88ccfddc0d6e5ee19 |
@drewbanin You have to give the full path to your project directory. |
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.
Just one comment, everything else looks good to me!
core/dbt/config/runtime.py
Outdated
@@ -353,7 +353,7 @@ def load_projects( | |||
yield project.project_name, project | |||
|
|||
def _get_project_directories(self) -> Iterator[Path]: | |||
root = Path(self.project_root) / self.modules_path | |||
root = Path.cwd() / self.modules_path |
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.
A little concern with possible behavior this line might introduce. I think the assumption here is that load_dependencies
will be called after moving to the root project dir. I think it is fine, will defer to you!
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.
LGTM! Thanks for hitting #2541 here also - that's a great change
resolves #2519
resolves #2541
Description
The problem was actually the opposite of what I thought:
dbt deps
does the wrong thing if you give it--project-dir
, and puts files relative to the current directory. But it does look in the--project-dir
to find packages.yml, so it just downloads them to the wrong directory.Subsequent
dbt
commands that do respect--project-dir
then can't find their dependencies, because they've moved into the project directory and away from the starting directory.Checklist
CHANGELOG.md
and added information about my change to the "dbt next" section.