-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
🎉 S3 source: add support for non-AWS S3 Storage #6398
Conversation
/test connector=connectors/source-s3
|
52e1910
to
79b4b08
Compare
79b4b08
to
583acba
Compare
583acba
to
42858c3
Compare
/test connector=connectors/source-s3
|
2511742
to
9bc12bb
Compare
9bc12bb
to
0bdf042
Compare
/test connector=connectors/source-s3
|
0bdf042
to
695515a
Compare
695515a
to
ac84381
Compare
/test connector=connectors/source-s3
|
status: "succeed" | ||
# for Parquet format | ||
- config_path: "secrets/parquet_config.json" | ||
status: "succeed" | ||
- config_path: "integration_tests/invalid_config.json" | ||
status: "failed" |
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.
Please leave at least one check with the result "failed"
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.
oops accidentally removed it
airbyte-integrations/connectors/source-s3/integration_tests/spec.json
Outdated
Show resolved
Hide resolved
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, small comments
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.
Awesome! Very cleanly implemented keeping things generic, like it!
Really just minor changes in my comments, main blocking point being the new methods sitting in source_files_abstract/__init__.py
airbyte-integrations/connectors/source-s3/acceptance-test-config.yml
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-s3/integration_tests/acceptance.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-s3/integration_tests/spec.json
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-s3/source_s3/source_files_abstract/__init__.py
Outdated
Show resolved
Hide resolved
@@ -81,7 +81,7 @@ def check_connection(self, logger: AirbyteLogger, config: Mapping[str, Any]) -> | |||
|
|||
found_a_file = False | |||
try: | |||
for filepath in self.stream_class.filepath_iterator(logger, config.get("provider")): | |||
for filepath in self.stream_class(**config).filepath_iterator(): |
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.
This was static so that during this connection check we wouldn't be instantiating an instance of the stream_class and running its init method, because our aim is purely to check connection.
I'm not against this change but curious as to why, I may be missing something.
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.
First I've made it non static cause I was storing endpoint not in "provider" but in separate "server" field and it was pain to pass all the configs first to the filepath_iterator and then to _list_backet static functions instead of just passing self. Than I merged "server" field into "provider" and decided to keep filepath_iterator function as instance method in case we would want to pass additional parameters to those functions in the future.
/test connector=connectors/source-s3
|
/publish connector=connectors/source-s3
|
What
Resolves #5751
How
Describe the solution
Recommended reading order
x.java
y.python
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/SUMMARY.md
docs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampledocs/integrations/README.md
airbyte-integrations/builds.md
Airbyter
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 hereUpdating a connector
Community member or Airbyter
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 hereConnector Generator
-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