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

AWS S3 and Alternative S3 destination connector feature #4038

Merged
merged 7 commits into from
Jun 13, 2021
Merged

AWS S3 and Alternative S3 destination connector feature #4038

merged 7 commits into from
Jun 13, 2021

Conversation

panhavad
Copy link
Contributor

What

Allow S3 Destination Connector have the option to connect to both standard AWS S3 and Minio S3(Alternative S3).
This PR answering to this issue #3900
image

Integration test result:
image

How

Because Minio S3 also supports AWS SDK, just use AwsClientBuilder.EndpointConfiguration to enable the custom endpoint.
Also check the condition, if the user enters 'aws' in the endpoint field. It will use the default AWS endpoint, else it connects to entered custom endpoint URL

Recommended reading order

  1. spec.json
  2. S3Consumer.java
  3. S3StreamCopier.java
  4. S3Config.java
  5. s3.md

Pre-merge Checklist

Expand the checklist which is relevant for this PR.

Connector checklist

  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • Secrets are annotated with airbyte_secret in output spec
  • Unit & integration tests added as appropriate (and are passing)
    • Community members: please provide proof of this succeeding locally e.g: screenshot or copy-paste acceptance test output. To run acceptance tests for a Python connector, follow instructions in the README. For java connectors run ./gradlew :airbyte-integrations:connectors:<name>:integrationTest.
  • /test connector=connectors/<name> command as documented here is passing.
    • Community members can skip this, Airbyters will run this for you.
  • Code reviews completed
  • Credentials added to Github CI if needed and not already present. instructions for injecting secrets into CI.
  • Documentation updated
    • README
    • CHANGELOG.md
    • Reference docs in the docs/integrations/ directory.
  • Build is successful
  • Connector version bumped like described here
  • New Connector version released on Dockerhub by running the /publish command described here
  • No major blockers
  • PR merged into master branch
  • Follow up tickets have been created
  • Associated tickets have been closed & stakeholders notified

Connector Generator checklist

  • Issue acceptance criteria met
  • PR name follows PR naming conventions
  • If adding a new generator, add it to the list of scaffold modules being tested
  • The generator test modules (all connectors with -scaffold in their name) have been updated with the latest scaffold by running ./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplates then checking in your changes
  • Documentation which references the generator is updated as needed.

@panhavad
Copy link
Contributor Author

@davinchia This is the new one, hope it fine. Feel free to suggest any change.

@davinchia davinchia requested a review from tuliren June 10, 2021 23:51
tuliren
tuliren suggested changes Jun 11, 2021
View reviewed changes
Copy link
Contributor

@tuliren tuliren left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you for the PR.

One suggestion is that s3_endpoint can just be an optional. What about defaulting to AWS when this field is empty? The rationales are: 1) asking people to type "aws" seems unnecessary, and 2) in this way, existing users of this connector does not need to migrate their config.

@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Jun 11, 2021
@panhavad
Copy link
Contributor Author

panhavad commented Jun 11, 2021 •
edited
Loading

Thank you @tuliren
Change has been made:
-Leave blank if using AWS
-By default blank
-s3_endpoint is not required
-PASSED integration test (Both AWS & Minio)
-PASSED build
-Tested on UI and API

@tuliren
Copy link
Contributor

tuliren commented Jun 13, 2021 •
edited by github-actions bot
Loading

/test connector=connectors/destination-s3

Error: No ref found for: contrib/alt-s3-destination-connector

tuliren
tuliren reviewed Jun 13, 2021
View reviewed changes
Copy link
Contributor

@tuliren tuliren left a comment

Choose a reason for hiding this comment

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

Thank you.

There is one NPE issue when s3_endpoint does not exist in the config (for existing S3 users). Look good and ready to merge otherwise.

panhavad and others added 2 commits June 13, 2021 14:29
@panhavad @tuliren
Update airbyte-integrations/connectors/destination-jdbc/src/main/java…
d0d37fa 
…/io/airbyte/integrations/destination/jdbc/copy/s3/S3Config.java

Co-authored-by: LiRen Tu <[email protected]>
@panhavad @tuliren
Update airbyte-integrations/connectors/destination-s3/src/main/java/i…
5b3b32c 
…o/airbyte/integrations/destination/s3/S3DestinationConfig.java

Co-authored-by: LiRen Tu <[email protected]>
panhavad
panhavad commented Jun 13, 2021
View reviewed changes
...estination-jdbc/src/main/java/io/airbyte/integrations/destination/jdbc/copy/s3/S3Config.java Outdated
@@ -74,7 +74,7 @@ public static S3Config getS3Config(JsonNode config) {
partSize = config.get("part_size").asInt();
}
return new S3Config(
config.get("s3_endpoint").asText(),
config.get("s3_endpoint") == null ? "" : config.get("s3_endpoint").asText()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tuliren did you forget ","

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, you are right.

panhavad
panhavad commented Jun 13, 2021
View reviewed changes
...destination-s3/src/main/java/io/airbyte/integrations/destination/s3/S3DestinationConfig.java Outdated
@@ -55,7 +55,7 @@ public S3DestinationConfig(

public static S3DestinationConfig getS3DestinationConfig(JsonNode config) {
return new S3DestinationConfig(
config.get("s3_endpoint").asText(),
config.get("s3_endpoint") == null ? "" : config.get("s3_endpoint").asText()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think the same here, "," missing

@tuliren tuliren merged commit d2f9643 into airbytehq:master Jun 13, 2021
@panhavad panhavad deleted the contrib/alt-s3-destination-connector branch June 13, 2021 07:59
@marcosmarxm marcosmarxm mentioned this pull request Jun 14, 2021
Merged
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tuliren tuliren tuliren approved these changes

Assignees
No one assigned
Labels
area/documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@panhavad @tuliren @marcosmarxm @Chensokheng