Install sqlalchemy-spanner package into Google provider#31925
Install sqlalchemy-spanner package into Google provider#31925potiuk merged 5 commits intoapache:mainfrom
Conversation
boring-cyborg
bot
added
area:providers
provider:google
phanikumv
left a comment
phanikumv
left a comment
There was a problem hiding this comment.
CI checks are failing, please fix them
|
@phanikumv I fixed tests, could you please check this PR one more time? |
whats the use case behind this change? What problem were you facing |
@phanikumv In this PR I have added ability to connect to Google Spanner Database using SQLAlchemy |
There was a problem hiding this comment.
I dont see an existing connection type gcpspanner, wondering how will this work ?
There was a problem hiding this comment.
@phanikumv it's a new connection. I have created it, because the google_cloud_default connection doesn't have an extra field. The extra field is needed for passing database_id and instance_id parameters. This connection will work the same like google_cloud_default
There was a problem hiding this comment.
if its just an extra field - can we think about adding it to google connection as an optional param rather than creating new connection altogether?
There was a problem hiding this comment.
Some time ago we had this extra field for extra parameters in the google_cloud_default connection, but then we decided for each extra parameter to create its own field. In my opinion it is not a good idea to return the extra field for google_cloud_default.
There was a problem hiding this comment.
Agree. Extra is well, dangerous. There were many CVEs issued recently because having access to edit extras would open all kinds of ways for those who have access to edit connections to do unintended things > Remote Code Execution, DOS, revealing extra information. We had to even reflect that in our security model and specifically flag the users who have connection editing capabilities as having to be trusted not to abuse those capabilities: https://airflow.apache.org/docs/apache-airflow/stable/security/index.html#capabilities-of-authenticated-ui-users
Generally speaking extras should be very limited and we maybe even should get rid of them at some point in time.
|
@phanikumv could you please approve my PR if everything looks good to you? |
There was a problem hiding this comment.
nitpick: Should we add a return type annotation here?
There was a problem hiding this comment.
@Lee-W it is an overridden method from extended class and in the extended class we also do not have type annotation. Also I have checked the create_engine function and this function also doesn't have type annotation. I don't know what type should be here.
There was a problem hiding this comment.
nitpick: Should we use it not all([instance_id, database_id])?
There was a problem hiding this comment.
@Lee-W thank you for the note I have changed this row.
MaksYermak
force-pushed
the
install-sqlalchemy-spanner
branch
3 times, most recently
from
176ad62 to
a6f3b9a
Compare
|
@potiuk hi, could you please look at this PR and merge it if everything is good? |
|
Rebase your PR please @MaksYermak |
MaksYermak
force-pushed
the
install-sqlalchemy-spanner
branch
3 times, most recently
from
4a30059 to
ff141ef
Compare
MaksYermak
force-pushed
the
install-sqlalchemy-spanner
branch
from
ff141ef to
f0795b4
Compare
5 participants
In this PR I have added sqlalchemy-spanner package to Google provider and updated the Google Spanner hook for supporting DbApiHook.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.