hussein-awala commented on PR #30175:
URL: https://github.com/apache/airflow/pull/30175#issuecomment-1509990359
> By all means but your inital comment boiled down to "i prefer this" and as
you point out this is to your knowledge at least an un documented convention.
Just to clarify, my initial comment was not based on a personal preference,
but rather on a convention or pattern used in other hooks. From what I
understand, the `get_conn` method should be responsible for establishing a
(cached) connection with the external service. Also, recent added hooks have
included a `__enter__` and `__exit__` method to use the hooks with the with
statement:
```python
def __enter__(self) -> SomeHook:
return self.get_conn()
def __exit__(self, exc_type, exc_val, exc_tb):
self.client.close()
```
> It also doesn't impact the external API or require a breaking change so I
think "we can revisit this in the future" is an appropriate response.
Regarding the breaking changes, once the provider is released, we cannot
delete the different methods that create the clients when we want to refacto
the method `get_conn`. Instead, we should deprecate them and remove them in the
next major version of the provider, as is done in other providers (here is an
ex from KPO
https://github.com/apache/airflow/pull/29498#discussion_r1167624973). So I'm
just trying to avoid deprecating methods in the first releases of the provider
by raising a small discussion.
The current blocker is the bug explained in
https://github.com/apache/airflow/pull/30175#discussion_r1167634983 (we can
discuss it if you think that there is no bug), which should be fixed before
merging this PR. However, merging the PR does not mean releasing the provider,
so even if we merge the PR we can still update the `get_conn` method (of course
there is a convention) in a separate PR before releasing the provider, but
there must be an agreement on this, hence the need for a small discussion in
this PR.
Sorry to insist on this subject, but I try to apply the rules explained in
this #30657:
> it might be enough to have a good and complete PR of the provider
your PR is great, I'm trying to make sure it's complete too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
