Skip to content
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

feat(source-gcs): added oauth flow #45414

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

darynaishchenko
Copy link
Collaborator

@darynaishchenko darynaishchenko commented Sep 12, 2024

What

Add supporting OAuth
Additionally:
resolves: https://github.com/airbytehq/airbyte-internal-issues/issues/9745 (remove discriminator fields)

How

Extended spec with advanced_auth, added credentials object with config migration that adds deprecated service_account field into credentials object.
Added and updated unit tests.

PR for the platform: https://github.com/airbytehq/airbyte-platform-internal/pull/13923

Review guide

  1. airbyte-integrations/connectors/source-gcs/source_gcs/config.py
  2. airbyte-integrations/connectors/source-gcs/source_gcs/source.py
  3. airbyte-integrations/connectors/source-gcs/source_gcs/stream_reader.py
  4. other files

User Impact

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

@darynaishchenko darynaishchenko self-assigned this Sep 12, 2024
Copy link

vercel bot commented Sep 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 24, 2024 2:07pm

@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Sep 13, 2024
@darynaishchenko darynaishchenko marked this pull request as ready for review September 13, 2024 13:35
"order": 0,
"type": "string"
"discriminator": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I expect the discriminator fields to trip up the Terraform provider. Maybe we need a method similar to this one where we remove them from the schemas

def remove_discriminator(schema: Dict[str, Any]) -> None:
"""pydantic adds "discriminator" to the schema for oneOfs, which is not treated right by the platform as we inline all references"""
dpath.delete(schema, "properties/**/discriminator")

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed def remove_discriminator from Config class, so both discriminators are being removed from spec

docs/integrations/sources/gcs.md Outdated Show resolved Hide resolved
Copy link
Contributor

@girarda girarda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks good! @darynaishchenko just to confirm - we need to update our app's scopes to be able to test the new flow, correct?

@darynaishchenko
Copy link
Collaborator Author

darynaishchenko commented Sep 17, 2024

PR looks good! @darynaishchenko just to confirm - we need to update our app's scopes to be able to test the new flow, correct?

@girarda, We need:

  1. merge pr for platform
  2. make a dev image of the source based changes in this pr
  3. add client id/secret (for example from other google app) to platform

Ask google support to test it for verification. Scopes have been already added to our app. cc: @DanyloGL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/gcs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants