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.

Sign up for GitHub

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

Jump to bottom

destination-s3: assume role auth #38204

Merged
merged 1 commit into from
May 17, 2024
Merged

destination-s3: assume role auth #38204

merged 1 commit into from
May 17, 2024

Conversation

stephane-airbyte
Copy link
Contributor

@stephane-airbyte stephane-airbyte commented May 15, 2024 •
edited
Loading

adding assume-role auth. We created a bunch of secrets in GSM, and allowed to pass an environment into the S3Destination constructor, so we don't modify the system env for everyone.

@vercel Vercel
Copy link

vercel bot commented May 15, 2024 •
edited
Loading

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 May 17, 2024 5:35pm

@stephane-airbyte Graphite App
Copy link
Contributor Author

stephane-airbyte commented May 15, 2024 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @stephane-airbyte and the rest of your teammates on Graphite Graphite

@stephane-airbyte stephane-airbyte mentioned this pull request May 15, 2024
Closed
@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues CDK Connector Development Kit connectors/destination/s3 labels May 15, 2024
@stephane-airbyte stephane-airbyte changed the title destination-s3: assume roal auth destination-s3: assume role auth May 15, 2024
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-s3_assume_roal_auth branch 2 times, most recently from 7cf3a63 to 58ef36f Compare May 15, 2024 03:47
@stephane-airbyte stephane-airbyte changed the base branch from connectors/destination/s3-assume-role-auth to master May 15, 2024 03:47
@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label May 15, 2024
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-s3_assume_roal_auth branch from 58ef36f to 12cbac5 Compare May 15, 2024 03:55
@stephane-airbyte stephane-airbyte mentioned this pull request May 15, 2024
Merged
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-s3_assume_roal_auth branch from 12cbac5 to 6c0962a Compare May 15, 2024 21:27
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-s3_assume_roal_auth branch from 6c0962a to 0870cd1 Compare May 15, 2024 22:12
@stephane-airbyte stephane-airbyte marked this pull request as ready for review May 15, 2024 22:15
@stephane-airbyte stephane-airbyte requested review from a team as code owners May 15, 2024 22:15
bgroff
bgroff approved these changes May 16, 2024
View reviewed changes
Copy link
Contributor

@bgroff bgroff left a comment

Choose a reason for hiding this comment

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

Thanks for all the hard work on this. Glad we were able to get it into good shape in the time we had.

edgao
edgao reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@edgao edgao left a comment

Choose a reason for hiding this comment

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

generally lgtm but had some clarifying questions

...kotlin/io/airbyte/cdk/integrations/destination/s3/credential/S3AssumeRoleCredentialConfig.kt Show resolved Hide resolved
...ns/src/test/kotlin/io/airbyte/cdk/integrations/destination/s3/credential/S3AssumeRoleTest.kt Outdated Show resolved Hide resolved
airbyte-integrations/connectors/destination-s3/src/main/resources/spec.json Outdated Show resolved Hide resolved
...on/java/io/airbyte/integrations/destination/s3/S3CsvAssumeRoleDestinationAcceptanceTest.java Outdated Show resolved Hide resolved
...ion-s3/src/test/java/io/airbyte/integrations/destination/s3/S3DestinationAssumeRoleTest.java Outdated Show resolved Hide resolved
...ion-s3/src/test/java/io/airbyte/integrations/destination/s3/S3DestinationAssumeRoleTest.java Outdated Show resolved Hide resolved
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-s3_assume_roal_auth branch from 8994a87 to 5f9b371 Compare May 16, 2024 22:12
@stephane-airbyte stephane-airbyte mentioned this pull request May 16, 2024
Closed
2 tasks
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-s3_assume_roal_auth branch from df6f812 to 55765d3 Compare May 16, 2024 22:53
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-s3_assume_roal_auth branch from 55765d3 to 60f5cb1 Compare May 16, 2024 23:00
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-s3_assume_roal_auth branch 2 times, most recently from 53569b5 to 9d2f7fb Compare May 17, 2024 00:53
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-s3_assume_roal_auth branch from 9d2f7fb to 30c2a33 Compare May 17, 2024 01:01
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-s3_assume_roal_auth branch from 30c2a33 to 116ae5e Compare May 17, 2024 01:52
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-s3_assume_roal_auth branch from 116ae5e to 9b3e09b Compare May 17, 2024 02:49
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-s3_assume_roal_auth branch from 9b3e09b to cb1ea02 Compare May 17, 2024 03:41
@stephane-airbyte Graphite App
Copy link
Contributor Author

@alafanechere I think the new secrets logic is broken for unit tests, unless I'm missing something obvious

@alafanechere
Copy link
Contributor

@alafanechere I think the new secrets logic is broken for unit tests, unless I'm missing something obvious

@stephane-airbyte the new secret logic is not yet in production. I only have seeded metadata files so far, and I'm actively working on changing the secret fetching logic in airbyte-ci.
The new secret you added might miss a connector and/or filename label in GSM so that ci_credentials can resolve it.

@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-s3_assume_roal_auth branch from cb1ea02 to be0d9d0 Compare May 17, 2024 16:47
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-s3_assume_roal_auth branch 2 times, most recently from 358b255 to 3268a41 Compare May 17, 2024 17:15
@stephane-airbyte stephane-airbyte force-pushed the stephane/05-14-destination-s3_assume_roal_auth branch from 3268a41 to 14edcb1 Compare May 17, 2024 17:31
@stephane-airbyte stephane-airbyte merged commit 91f1862 into master May 17, 2024
29 checks passed
@stephane-airbyte stephane-airbyte deleted the stephane/05-14-destination-s3_assume_roal_auth branch May 17, 2024 17:52
@stephane-airbyte Graphite App
Copy link
Contributor Author

Merge activity

@evantahler
Copy link
Contributor

Linking the Loom Demo https://www.loom.com/share/225fe3aa4bab4f379f3f4af99cb425e3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bgroff bgroff bgroff approved these changes

@edgao edgao edgao approved these changes

@gisripa gisripa Awaiting requested review from gisripa

Assignees
No one assigned
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation CDK Connector Development Kit connectors/destination/s3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@stephane-airbyte @alafanechere @evantahler @bgroff @edgao @octavia-squidington-iii