-
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
Fix dbt deps failing on tarballs #9068
Conversation
This test was written _after_ the fix was commited. However, I ran this test against main without the fix and it failed. After running the test with the tarball fix, it passed.
…date` We had a conditional to skip validation for a package if the package included the `tarball` key. However, this conditional always returned false as it was nested inside a conditional that the package had the default `package` key, which means it's not a tarball package, but a package package (maybe we need better differentiation here). If we need additional validation for tarballs down the road, we should do that one level up. At this time we have no additional validaitons to add.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9068 +/- ##
==========================================
+ Coverage 86.56% 86.60% +0.03%
==========================================
Files 179 179
Lines 26538 26538
==========================================
+ Hits 22973 22983 +10
+ Misses 3565 3555 -10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Thanks for removing the logic that's not needed! |
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.
Small fix to the changelog, otherwise it looks good.
We initially included this fixture due to copy and pasting another test. However, this `setUp` fixture isn't actually necessary for the tarball dependency tests.
* tarball lockfile fix * Add changie doc for tarball deps issue * Add integration test for ensuring tarball package specification works This test was written _after_ the fix was commited. However, I ran this test against main without the fix and it failed. After running the test with the tarball fix, it passed. * Remove unnecessary `tarball` conditional logic in `PackageConfig.validate` We had a conditional to skip validation for a package if the package included the `tarball` key. However, this conditional always returned false as it was nested inside a conditional that the package had the default `package` key, which means it's not a tarball package, but a package package (maybe we need better differentiation here). If we need additional validation for tarballs down the road, we should do that one level up. At this time we have no additional validaitons to add. * Fix typos in changie doc for tarball deps issue * Improve tarball package test naming and add related unhappy path test * Remove unnecessary `setUp` fixture from tarball package tests We initially included this fixture due to copy and pasting another test. However, this `setUp` fixture isn't actually necessary for the tarball dependency tests. --------- Co-authored-by: Chenyu Li <chenyu.li@dbtlabs.com> (cherry picked from commit 017faf4)
resolves #9062
Problem
In 1.7.x it's default behavior for a
packages-lock.yml
to be produced when runningdbt deps
. However, ourto_dict
function on tarball packages tries to produces fields which don't actually exist (and are thus populated withNone
), and drop some of the required fields 😬 Thus when we try to parse thepackages-lock.yml
the process blows up 😅 Specifically it raises aDbtProjectError
saying thepackages.yml
is malformed. However the error is really that we mangled thepackages-lock.yml
, and thus the yaml can't be used to produce the python object representation of the tarball package.Solution
We corrected the
to_dict
function for tarball packages.Checklist