Pass credentials directly to GCP connectors rather than through environment variable

move-coop / parsons

A python library of connectors for the progressive community.

Other

254 stars 125 forks source link

Pass credentials directly to GCP connectors rather than through environment variable #1040

Open austinweisgrau opened 2 months ago

austinweisgrau commented 2 months ago

Pass credentials directly to GCP connectors rather than through shared environment variable. Avoids issue documented in move-coop#1039 where credentials for all GCP clients are stored in the same environment variable, leading to overwrites if multiple clients are initialized in the same environment.

TODO

[x] Fix for BigQuery connector
[x] Test BigQuery connector works with live creds
[x] Fix for Sheets connector
[x] Test Sheets connector works with live creds
[x] Fix for Admin connector
[x] Test Admin connector works with live creds
[x] Fix for CloudStorage connector
[x] Test CloudStorage connector works with live creds
[x] Fix for tests

austinweisgrau commented 2 months ago

Ok this should qualify as a true refactor, where no existing behavior is altered (except for a very hacky use of these classes), but the bug will go away.

austinweisgrau commented 2 months ago

I haven't used the Admin connector - curious if someone else can test that. ... @KasiaHinkson have u used it?

austinweisgrau commented 2 months ago

Just ensuring that it can authenticate is all thats needed

KasiaHinkson commented 1 month ago

I have not used the Admin connector and this has been sitting in my inbox too long waiting for me to find the time to explore that and see if I could test it. I can go ahead and review it assuming that since this works for other connectors it will presumably work for the Admin connector, or I can keep waiting and try to make time next week?

austinweisgrau commented 1 month ago

So, funny thing. I enabled the Google Admin API so I could test this out.

Turns out, the changes in this PR did break the parsons GoogleAdmin connector, a small change in argument order on the new client request method. That is fixed here.

Trouble is, this connector was actually already broken. I tested the connector on main and its methods return empty Tables every time. Digging in, every request returns an error, but the connector doesn't check for that and just ends up returning empty Tables.

So the connector is actually still as broken as it was on main, new issue here #1058 to track that. This PR should be good to go though - the new authentication works as intended.