Skip to content

Add package 'name' to lock file #11488

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

Merged
merged 3 commits into from
Apr 15, 2025
Merged

Add package 'name' to lock file #11488

merged 3 commits into from
Apr 15, 2025

Conversation

jtcohen6
Copy link
Contributor

Resolves #11487

Problem

dbt gets the true project name for each configured package (and their dependencies), but it doesn't store that information anywhere, requiring multiple trips to git providers / hub packages.

Solution

  • Write each package's true project name in package-lock.yml
  • Without changing the sha1_hash (if possible)

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 or 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.
  • This PR includes type annotations for new and modified functions.

Sorry, something went wrong.

@jtcohen6 jtcohen6 requested a review from a team as a code owner April 11, 2025 00:06
@cla-bot cla-bot bot added the cla:yes label Apr 11, 2025
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.

@zqureshi
Copy link
Member

👀

Copy link

codecov bot commented Apr 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.88%. Comparing base (7cca847) to head (7da1e0a).
Report is 2 commits behind head on main.

❌ Your patch status has failed because the patch coverage (53.84%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #11488   +/-   ##
=======================================
  Coverage   88.88%   88.88%           
=======================================
  Files         194      194           
  Lines       24553    24564   +11     
=======================================
+ Hits        21824    21834   +10     
- Misses       2729     2730    +1     
Flag Coverage Δ
integration 85.88% <100.00%> (+<0.01%) ⬆️
unit 62.77% <53.84%> (-0.01%) ⬇️

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

Components Coverage Δ
Unit Tests 62.77% <53.84%> (-0.01%) ⬇️
Integration Tests 85.88% <100.00%> (+<0.01%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@@ -42,13 +42,19 @@ class Quoting(dbtClassMixin, Mergeable):

@dataclass
class Package(dbtClassMixin):
pass

def __post_serialize__(self, data, context: Optional[Dict]):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The intent is to avoid changing the sha1_hash in package-lock.yml just because there's now an optional name attribute in the contract:

{'git': 'https://github.com/dbt-labs/dbt-utils', 'revision': None, 'warn-unpinned': None, 'subdirectory': None, 'unrendered': {'git': 'https://github.com/dbt-labs/dbt-utils'}, 'name': None}

But name actually is a required field for TarballPackage

Copy link
Member

Choose a reason for hiding this comment

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

This would be great to leave as a comment for the method.

Copy link
Contributor Author

@jtcohen6 jtcohen6 Apr 11, 2025

Choose a reason for hiding this comment

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

I can definitely add a comment. I'm also open to moving this logic elsewhere, so that the intent is clearer and the impact is narrower - perhaps task.deps._create_sha1_hash, where package.to_dict is actually being called?



@dataclass
class LocalPackage(Package):
local: str
unrendered: Dict[str, Any] = field(default_factory=dict)
name: Optional[str] = None
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the deps task here, dbt reads package-lock.yml and validates its contents using these contracts. So if name is in there, it needs to be an allowed (optional) field in all Package contracts.

The alternative would be to strip out name when dbt reads that file — but leave it in for TarballPackage only? — which feels trickier.

Copy link
Member

@zqureshi zqureshi left a comment

Choose a reason for hiding this comment

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

LGTM but I would have @peterallenwebb or @emmyoop rubber stamp since they are doing infra things.

@@ -204,7 +207,7 @@ def run(self) -> None:
if self.args.add_package:
self.add()

# Check lock file exist and generated by the same pacakges.yml
# Check lock file exist and generated by the same packages.yml
Copy link
Member

Choose a reason for hiding this comment

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

I was wondering why this is a diff... until I saw it.

self.project.packages.packages
)
package_dict = package.to_dict()
package_dict["name"] = package.get_project_name(self.project, renderer)
Copy link
Member

Choose a reason for hiding this comment

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

@@ -34,9 +34,11 @@ def test_deps_lock(self, clean_start):
with open("package-lock.yml") as fp:
contents = fp.read()

fivetran_package = "- package: fivetran/fivetran_utils\n version: 0.4.7"
fivetran_package = (
Copy link
Member

Choose a reason for hiding this comment

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

nit: I wonder if this would be clearer to read with a heredoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

qq chose comma ça ?

        fivetran_package = """
  - name: fivetran_utils
    package: fivetran/fivetran_utils
    version: 0.4.7
"""

Copy link
Member

Choose a reason for hiding this comment

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

If you also feel that it increases comprehension, I'm 50/50.

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 do think it's easier to read!

@jtcohen6 jtcohen6 merged commit 9c91ab2 into main Apr 15, 2025
60 of 61 checks passed
@jtcohen6 jtcohen6 deleted the jerco/impl-11487 branch April 15, 2025 05:19
@zqureshi
Copy link
Member

🚀🚀🚀

QMalcolm pushed a commit that referenced this pull request Apr 17, 2025
* Add package 'name' to lock file

* PR feedback + changie

* Fix test + add comment
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.

[Impl] Include each package's project name in package-lock.yml
3 participants