-
Notifications
You must be signed in to change notification settings - Fork 175
Try dbt-core#7368 #774
Try dbt-core#7368 #774
Conversation
|
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 dbt-bigquery contributing guide. |
setup.py
Outdated
| "google-cloud-storage~=2.4", | ||
| "google-cloud-dataproc~=5.0", | ||
| "agate~=1.6.3", | ||
| "agate~=1.7.0", |
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.
Is there a reason why agate is an explicit dependency here, rather than just using the transitive dependency from dbt-core?
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.
@colin-rogers-dbt Looks like you added this back in #458 (d27cf24)
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.
I can't remember why, if it's causing issues we can certainly remove it
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.
We should keep agate if we're using it directly, but without a pin.
We ran into an issue a few months ago where a requirement was dropped from dbt-core that was used directly in an adapter. To avoid version conflicts, we did not explicitly list it as a requirement in the adapter; hence the adapter broke when we removed it from dbt-core. This is likely a rare occurrence, but I think this is the right compromise with the least downsides. I did that here in dbt-redshift:
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.
@mikealfare Heard! I removed it in #778; shall I open a new PR to add it back (without a pin)?
2776a01 to
45c5757
Compare
|
I'll open the removal of the |
Run CI with updated dependency pins