Drop Connection.schema use in DbtCloudHook #29166
Conversation
| :param endpoint: Endpoint url to be requested. | ||
| :param include_related: Optional. List of related fields to pull with the run. | ||
| Valid values are "trigger", "job", "repository", and "environment". | ||
| """ | ||
| data: dict[str, Any] = {} | ||
| base_url = f"https://{tenant}.getdbt.com/api/v2/accounts/" |
There was a problem hiding this comment.
Old logic that is no longer valid.
| if base_url and not base_url.endswith("/") and endpoint and not endpoint.startswith("/"): | ||
| url = base_url + "/" + endpoint | ||
| else: | ||
| url = (base_url or "") + (endpoint or "") |
There was a problem hiding this comment.
Refactoring the branching here since the else would never be executed anyway.
josh-fell
force-pushed
the
dbt-cloud-fix-tenant-async
branch
2 times, most recently
from
69b599c
to
05ea8e3
Compare
| @staticmethod | ||
| def _get_tenant_domain(conn: Connection) -> str: | ||
| if conn.schema: | ||
| warnings.warn( |
There was a problem hiding this comment.
We can make a major release with the updated behavior rather than carry the deprecation further
There was a problem hiding this comment.
Done. Do you think this warrants a newsfragment or does the major version increment imply something "newsworthy"?
There was a problem hiding this comment.
You will need to update version in provider.yaml to 3.0.0 and also update provider changelog with details of the breaking change (check Amazon provider change log for example)
There was a problem hiding this comment.
Oh right 🤦. I updated these now along with the PR title and description to make it clear what the significant change is in this PR.
josh-fell
force-pushed
the
dbt-cloud-fix-tenant-async
branch
2 times, most recently
from
663fdb7
to
c1cb663
Compare
josh-fell
changed the title
Related: apache#28890 apache#29014 There was a recent enhancement of DbtCloudRunJobOperator to include deferrable/async functionality. Unfortunately the `tenant` evaluation in the DbtCloudHook was outdated and didn't include the most recent change to properly handle domain specification. This PR consolidates the tenant eval logic to a common method to be used by both sync and async methods in the hook.
josh-fell
force-pushed
the
dbt-cloud-fix-tenant-async
branch
from
c1cb663
to
7f1647a
Compare
Related: #28890 #29014
In version 2.3.1 of this provider, an fix was made to allow users to specify the entire tenant domain on their dbt Cloud instance rather than just the third-level (e.g. "my-tenant.domain.com" vs. "my-tenant.getdbt.com") via Connection.host. This was particular useful for other dbt Cloud regions other than the US. Backwards compat was still available via Connection.schema, but was noted as deprecated. Given other changes are being made to the provider, Connection.schema use to specify the dbt Cloud tenant domain should be dropped.
Also, there was a recent enhancement of DbtCloudRunJobOperator to include deferrable/async functionality. Unfortunately the
tenantevaluation in the DbtCloudHook was outdated and didn't include the most recent change to properly handle domain specification. This PR consolidates the tenant eval logic to a common method to be used by both sync and async methods in the hook.