add google sheets oauth flow to server + fix auth rootObject type in protocol #7131
Conversation
| public class GoogleSheetsOAuthFlow extends GoogleOAuthFlow { | ||
|
|
||
| @VisibleForTesting | ||
| static final String SCOPE_URL = "https://www.googleapis.com/auth/spreadsheets.readonly"; |
There was a problem hiding this comment.
shouldn't the scopes also include Gdrive readonly like listed here?
There was a problem hiding this comment.
I left out Gdrive because get_authenticated_drive_client() is never actually used but I'll add it in for completeness.
| @@ -227,7 +227,7 @@ definitions: | |||
| " | |||
| type: array | |||
| items: | |||
| type: string | |||
| type: [string, integer] | |||
There was a problem hiding this comment.
this will require also CDK models and releasing a version of the CDK, also updating the API interface since it returns the same struct, and testing inserting the conversion between the two in the tests for this class
There was a problem hiding this comment.
gotcha. I believe this is a correct change and therefore worth the additional changes, sheets connector is hitting jsonvalidationerror on get spec otherwise, understandably because of the index integer in this array.
I'm not entirely sure why other connectors with an integer aren't already failing though... 🤷
Thanks for highlighting required changes!
| assertTrue(credentials.get("access_token").toString().length() > 0); | ||
| } | ||
|
|
||
| static class ServerHandler implements HttpHandler { |
There was a problem hiding this comment.
would it make sense to DRY this component? feels like we use it in every test class
There was a problem hiding this comment.
yes all oauth backends are going to copy/paste this test structure, maybe worth DRYing it and making it easier for #4982
There was a problem hiding this comment.
Agreed, I think there's a lot of DRY to be had on the testing but thought I'd avoid until #4982 was clear in case the effort would be made redundant. If these will likely exist as is regardless then I'll DRY as part of this PR.
# Conflicts: # airbyte-oauth/src/main/java/io/airbyte/oauth/OAuthImplementationFactory.java
github-actions
bot
added
area/api
Phlair
changed the title
|
/publish-cdk dry-run=true
|
|
/publish-cdk dry-run=false
|
|
Merging this and will create a separate PR for bumping OSS Airbyte version |
…protocol (airbytehq#7131) * adding google sheets oauth flow to server * fix oauth type in protocol yaml * bump sheets version in definitions * added GDrive scope * update sheets to master changes * update protocol incl. cdk * protocol typing for oauth rootobject * format
What
starting point for: #7038
Confirmed
Sheets,AdsandSearch ConsoleOAuth flow all working locally, ready to deploy and record demo videos.Also, in protocol, updated type of OAuth2Specification.rootObject to array of [string or integer] to accurately reflect holding string keys and/or array index.
Note
purposely avoided DRYing tests as it could be made redundant by work that gets done next in this issue