VladaZakharova commented on PR #56324:
URL: https://github.com/apache/airflow/pull/56324#issuecomment-3385131199
@Ankurdeewan Hi there!
Thank you for the changes, I think it is really handy
I have only several concerns about the implementation itself
- the approach with writing system tests as unit tests with "_system.py"
prefix is a very old approach, please check other PR to google provider for the
examples how to do that
- as I can see you have created new folder for the documentation for this
new parameter you have introduced. This shouldn't be in the new file, instead I
would suggest moving this docs to the one that is connected to GoogleBaseHook
documentation providers/Google/docs/connections/GCP.rst file.
- I think there is no need to create any new folder, like "/common" for
everything connected to this. Instead, you can move also unit tests to the file
that already exists: test_base_google.py file
- here ->
https://github.com/apache/airflow/pull/56324/files#diff-8982cd72c0faea336f97868e0f69198e2a5b86b9e686a32db6a4b220cb7b1459R419
- this method raises exception if it is not implemented. and we have an option
when user can create anonymous credentials and for this type of creds you also
use this credentials. So maybe you can add some logic for this case too? With
anonymous = True and quota_project_id = "blabla" rn your code will raise an
exception, right?
--
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]
