ashb commented on pull request #8974:
URL: https://github.com/apache/airflow/pull/8974#issuecomment-644739992
> I see a few options of using Backend <> Hook in this scenario:
>
> 1. we build an artificial connection and pass it to hook (very bad
idea IMHO)
Agreed, that sounds bad.
>
> 2. Get Secret to use Hook directly. For that we will have to change
the Hook to accept the basic parameters (host/port etc.) not only from
connection but also if there is a lack of connection it should have additional
parameters for host, port, user, passwords (or maybe just url) from the
parameters passed. I believe this is what you propose @ashb ? Correct?
I was picturing just a URL (given that is how we configure the secrets
backend now. Well URL + kwargs)
>
> 3. Implement intermediary used by both Hook and Secret - (Client in my
case) which is library used by both - Hook and Backend. Client has no relation
to Connection - it's Hook's job to provide all the parameters to the Client
parsed from the connection.
I think my reservation to 3 was that this is a new concept to Airflow and it
hadn't been mentioned before, nor was the reasoning for having this new class
made clear. I should have been better about asking for that reason than jumping
straight to my -1 comment.
Equally, there was nothing in the PR or (doc) comments explaining why which
means I had to guess or come to my own (wrong) conclusions.
> For 2), I think exposing password/URL/login via Hook constructor is a bad
idea. The hook should encourage connection as the only source of the
connectivity information - because it is encrypted and shared with other
workers by design.
Yes I agree. (I don't even like the pattern where AWS hooks accept an
optional "region_name" parameter to some functions. Separate thing, but related
point.)
> Is the problem with documentation duplication? I think the separation of
concerns is fairly solid here.
On reflection I hadn't thought through my suggestion fully, but my objection
was based around not setting precedent -- that is to not add a new "concept"
inside a provider. But your points make sense, and Hook should only operate
from connection info, not accept H+U+P directly.
How about calling it
airflow.providers.hashicorp._internal.client.VaultClient? That would address my
concerns about a new concept. And I think this concern only comes in to play
with (some) Secrets backends, but not for most other providers, which would
just have a hook and not need this extra client class. Could we perhaps also
add this to a comment in where ever this class ends up living.
I also think this "client" class should be excluded from the generated API
docs -- that makes it not part of the public API.
I'm really sorry for making you do the extra work of moving the class (not
to mention the time you had to spend writing the comment.)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
