-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🎉 Source Google Sheets: Support connecting via oAuth webflow #6354
Conversation
/test connector=connectors/source-google-sheets
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but requires some improvements.
@@ -4,18 +4,81 @@ | |||
"$schema": "http://json-schema.org/draft-07/schema#", | |||
"title": "Stripe Source Spec", | |||
"type": "object", | |||
"required": ["spreadsheet_id", "credentials_json"], | |||
"additionalProperties": false, | |||
"required": ["spreadsheet_id"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is "credentials" not required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the idea is to have a backward compatibility with the old-style config, and credentials are still required for auth through the UI
"refresh_token" | ||
], | ||
"properties": { | ||
"auth_type": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, auth_type shouldn't be part of specification. It should be recognized in code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vitaliizazmic this practice is used in the others connectors, could you point to some code with a new approach?
} | ||
} | ||
}, | ||
"authSpecification": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What this for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on #6456
"client_secret": "invalid_secret", | ||
"refresh_token": "invalid_token" | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you format?
if credentials.get("auth_type") == "Service": | ||
return service_account.Credentials.from_service_account_info(json.loads(credentials["service_account_info"]), scopes=scopes) | ||
elif credentials.pop("auth_type") == "Client": | ||
return client_account.Credentials.from_authorized_user_info(info=credentials) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might be wrong, but what will happen here if the first comparison fail?
so the auth_type
is already removed, would it always return None?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I read it as a two pop
s, but nevertheless can't we pop auth_type
first and then do if/else logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see comments
/test connector=connectors/source-google-sheets
|
/test connector=connectors/source-google-sheets
|
/test connector=connectors/source-google-sheets
|
/publish connector=connectors/source-google-sheets
|
…hq#6354) * Add service account support * Upd oauth support * Upd auth creds selector
What
Add support for connecting via Ouath webflow
How
Update spec.json schema to support 2 types of authorization
Update google_sheets_source.py, get_credentials method which returns credentials for a specific authorization method
Update helpers.py, add logic for choosing authorization type
Pre-merge Checklist
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing./publish
command described here