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

add typing to DepsTask.run #6192

Merged
merged 1 commit into from
Nov 3, 2022
Merged

add typing to DepsTask.run #6192

merged 1 commit into from
Nov 3, 2022

Conversation

MichelleArk
Copy link
Contributor

@MichelleArk MichelleArk commented Nov 2, 2022

resolves #6191

Description

fire_event(DepsNotifyUpdatesAvailable(packages=packages_to_upgrade)) was previously passing packages as a list of strings which could not be serialized to the new ListOfStrings protobuf type during logging.

dbt --log-format json deps logged the following error event when installing a package that has an update available:

{"exc": "type DepsNotifyUpdatesAvailable is not serializable. 'str' object has no attribute 'to_dict'", "info": {"code": "Z002", "extra": {}, "invocation_id": "1e59a3df-3a23-42d9-aa45-d2246092cd70", "level": "error", "msg": "Encountered an error:\ntype DepsNotifyUpdatesAvailable is not serializable. 'str' object has no attribute 'to_dict'", "name": "MainEncounteredError", "pid": 51212, "thread": "MainThread", "ts": "2022-11-02T00:02:17.987107Z"}}

Ideally mypy would have caught this issue but DepsTask.run was not scanned by mypy because the method signature did not have type annotations. After adding type annotations to the DepsTask.run, mypy was able to detect the issue (as well as a few others which are also addressed in this change):

❯ mypy core/dbt/task/deps.py
core/dbt/task/deps.py:90: error: Argument "packages" to "DepsNotifyUpdatesAvailable" has incompatible type "List[str]"; expected "ListOfStrings"
Found 1 error in 1 file (checked 1 source file)

Checklist

@cla-bot cla-bot bot added the cla:yes label Nov 2, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022

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.

@MichelleArk MichelleArk added the Skip Changelog Skips GHA to check for changelog file label Nov 2, 2022
@MichelleArk MichelleArk marked this pull request as ready for review November 2, 2022 19:36
@MichelleArk MichelleArk requested a review from a team November 2, 2022 19:36
@MichelleArk MichelleArk requested review from a team as code owners November 2, 2022 19:36
Copy link
Contributor

@iknox-fa iknox-fa left a comment

Choose a reason for hiding this comment

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

LGTM

@leahwicz leahwicz requested a review from gshank November 3, 2022 17:35
@MichelleArk MichelleArk merged commit cfad27f into main Nov 3, 2022
@MichelleArk MichelleArk deleted the arky/CT-1461-deps-typing branch November 3, 2022 21:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla:yes Skip Changelog Skips GHA to check for changelog file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CT-1461] [Bug] DepsNotifyUpdatesAvailable is not serializable
3 participants