✨ Source S3: Add handling NoSuchBucket error #31383

tolik0 · 2023-10-13T12:18:03Z

What

Add raising config error in case of wrong bucket name.
Closes:
#25835

How

Added a new type of error to file-based cdk to avoid catching in file-based cdk and just raise AirbyteTracedException config error.

vercel · 2023-10-13T12:18:10Z

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

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	⬜️ Ignored (Inspect)	Visit Preview		Oct 24, 2023 7:52pm

github-actions · 2023-10-13T12:18:25Z

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

PR name follows PR naming conventions
Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan.
Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
You've updated the connector's metadata.yaml file any other relevant changes, including a breakingChanges entry for major version bumps. See metadata.yaml docs
Secrets in the connector's spec are annotated with airbyte_secret
All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
Migration guide updated in docs/integrations/<source or destination>/<name>-migrations.md with an entry for the new version, if the version is a breaking change. See migration guide example
If set, you've ensured the icon is present in the platform-internal repo. (Docs)

If the checklist is complete, but the CI check is failing,

Check for hidden checklists in your PR description
Toggle the github label checklist-action-run on/off to re-run the checklist CI.

airbyte-oss-build-runner · 2023-10-13T12:34:33Z

source-s3 test report (commit `75069a5c82`) - ❌

⏲️ Total pipeline duration: 28.60s

Step	Result
Build source-s3 docker image for platform(s) linux/x86_64	❌
Code format checks	✅
Validate metadata for source-s3	✅
Connector version semver check	✅
Connector version increment check	✅
QA checks	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

clnoll · 2023-10-16T22:32:29Z

...yte_cdk/sources/file_based/availability_strategy/default_file_based_availability_strategy.py

@@ -64,6 +64,8 @@ def check_availability_and_parsability(
    def _check_list_files(self, stream: "AbstractFileBasedStream") -> List[RemoteFile]:
        try:
            files = stream.list_files()
+        except CustomFileBasedSourceException as custom_exc:


@tolik0 thanks for this PR, I agree that we want to be able customize the exceptions.

Looking at this code: the error that's relevant to the user is still that there was a problem listing files, so I think we want to keep the CheckAvailabilityError, but want to make sure we can customize the message, e.g. for things like missing bucket.

Since we're already raising CheckAvailabilityError ... from exc, we should get the missing bucket error as part of the stacktrace when we raise the CheckAvailabilityError, right?

If that's obscuring the error too much, instead of raising CustomFileBasedSourceException, WDYT about giving it a "message" attribute, then here we'll raise CheckAvailabilityError(custom_exc.message, stream=stream.name) from exc?

@clnoll Thanks for feedback!
I've removed CustomFileBasedSourceException. Do you think this is better? Should I split this PR into two: one for file-based CDK and another for S3 changes?

clnoll

Thanks @tolik0! I think I'm actually preferring to keep the CustomFileBasedSourceException. I left an example of what I have in mind in a comment.

I think you will have to split this into 2 PRs since the new CDK version won't be available yet when the source-s3 connector is published.

clnoll · 2023-10-18T22:37:33Z

...yte_cdk/sources/file_based/availability_strategy/default_file_based_availability_strategy.py

@@ -71,7 +71,8 @@ def _check_list_files(self, stream: "AbstractFileBasedStream") -> List[RemoteFil
        try:
            files = stream.list_files()
        except Exception as exc:
-            raise CheckAvailabilityError(FileBasedSourceError.ERROR_LISTING_FILES, stream=stream.name) from exc


I think what I'd do here is continue to raise the CustomException from your previous version, but catch it and re-raise with a CheckAvailabilityError, like this:

try: ... except CustomFileBasedException as exc: raise CheckAvailabilityError(exc.error_message, stream=stream.name) from exc except Exception as exc: ...

This way we can avoid having to do the hasattr check. Plus, I think we'll probably want to use your CustomFileBasedException in other places too as the file-based CDK matures.

I've updated the code to use CustomFileBasedException and re-raise as CheckAvailabilityError, removing the hasattr check. Thanks for the suggestion.

clnoll · 2023-10-18T22:40:51Z

airbyte-integrations/connectors/source-s3/source_s3/v4/stream_reader.py

-                        yield remote_file
-            else:
-                for remote_file in self._page(s3, globs, self.config.bucket, None, seen, logger):
+            for current_prefix in (prefixes if prefixes else [None]):


Why is this change needed?

I simplified the code to avoid duplication

clnoll · 2023-10-18T22:41:17Z

airbyte-integrations/connectors/source-s3/source_s3/v4/stream_reader.py

-            ) from exc
+            self.raise_error_listing_files(FileBasedSourceError.ERROR_LISTING_FILES, globs, exc)
+
+    def raise_error_listing_files(self, error_message: Union[str, FileBasedSourceError], globs: List[str], exc: Optional[Exception] = None):


Why do we need a helper method for this?

I used the helper method to reduce repetitive error-raising code.

airbyte-oss-build-runner · 2023-10-18T22:57:45Z

source-s3 test report (commit `c60529b0df`) - ❌

⏲️ Total pipeline duration: 16mn30s

Step	Result
Build source-s3 docker image for platform(s) linux/x86_64	✅
Unit tests	✅
Acceptance tests	❌
Code format checks	✅
Validate metadata for source-s3	✅
Connector version semver check	✅
Connector version increment check	✅
QA checks	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

airbyte-oss-build-runner · 2023-10-19T00:03:11Z

source-s3 test report (commit `3c18702032`) - ❌

⏲️ Total pipeline duration: 02mn36s

Step	Result
Build source-s3 docker image for platform(s) linux/x86_64	❌
Code format checks	❌
Validate metadata for source-s3	✅
Connector version semver check	✅
Connector version increment check	✅
QA checks	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

airbyte-oss-build-runner · 2023-10-19T00:15:15Z

source-s3 test report (commit `30e354ac81`) - ❌

⏲️ Total pipeline duration: 14.54s

Step	Result
Build source-s3 docker image for platform(s) linux/x86_64	❌
Code format checks	✅
Validate metadata for source-s3	✅
Connector version semver check	✅
Connector version increment check	✅
QA checks	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

clnoll

Thanks @tolik0, looks great!

airbyte-oss-build-runner · 2023-10-23T11:23:26Z

source-s3 test report (commit `3edc8af363`) - ❌

⏲️ Total pipeline duration: 03mn22s

Step	Result
Build source-s3 docker image for platform(s) linux/amd64	❌
Code format checks	❌
Validate metadata for source-s3	✅
Connector version semver check	✅
Connector version increment check	✅
QA checks	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

airbyte-oss-build-runner · 2023-10-23T11:38:56Z

source-s3 test report (commit `0550b77564`) - ❌

⏲️ Total pipeline duration: 03mn29s

Step	Result
Build source-s3 docker image for platform(s) linux/amd64	❌
Code format checks	✅
Validate metadata for source-s3	✅
Connector version semver check	✅
Connector version increment check	✅
QA checks	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

airbyte-oss-build-runner · 2023-10-24T20:12:28Z

source-s3 test report (commit `fe9d87a2fd`) - ✅

⏲️ Total pipeline duration: 17mn56s

Step	Result
Build source-s3 docker image for platform(s) linux/amd64	✅
Unit tests	✅
Acceptance tests	✅
Check our base image is used	✅
Code format checks	✅
Validate metadata for source-s3	✅
Connector version semver check	✅
Connector version increment check	✅
QA checks	✅

🔗 View the logs here

☁️ View runs for commit in Dagger Cloud

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-s3 test

octavia-squidington-iii added area/connectors Connector related issues area/documentation Improvements or additions to documentation CDK Connector Development Kit labels Oct 13, 2023

octavia-squidington-iii added the connectors/source/s3 label Oct 13, 2023

tolik0 assigned roman-yermilov-gl Oct 13, 2023

tolik0 marked this pull request as ready for review October 13, 2023 12:32

tolik0 requested a review from a team as a code owner October 13, 2023 12:32

tolik0 unassigned roman-yermilov-gl Oct 13, 2023

tolik0 requested a review from roman-yermilov-gl October 13, 2023 12:32

girarda requested a review from clnoll October 13, 2023 16:53

clnoll reviewed Oct 16, 2023

View reviewed changes

tolik0 force-pushed the tolik0/source-s3/wrong-bucket-config-error branch from 9f586f9 to db3ff06 Compare October 18, 2023 22:24

octavia-squidington-iv requested a review from a team October 18, 2023 22:25

tolik0 requested a review from clnoll October 18, 2023 22:29

clnoll reviewed Oct 18, 2023

View reviewed changes

tolik0 force-pushed the tolik0/source-s3/wrong-bucket-config-error branch from c60529b to 3c18702 Compare October 18, 2023 23:59

clnoll approved these changes Oct 19, 2023

View reviewed changes

bazarnov approved these changes Oct 19, 2023

View reviewed changes

tolik0 mentioned this pull request Oct 23, 2023

🎉 Airbyte CDK: Add CustomFileBasedException for custom errors in file-based CDK #31704

Merged

tolik0 and others added 5 commits October 23, 2023 14:15

Add handling NoSuchBucket error

d9bce94

Automated Commit - Formatting Changes

14e5b9e

Refactor custom error raising

9ad2d20

Add CustomFileBasedException

6eeb47f

Delete changes to file-based CDK

3edc8af

tolik0 force-pushed the tolik0/source-s3/wrong-bucket-config-error branch from 30e354a to 3edc8af Compare October 23, 2023 11:17

octavia-squidington-iii removed the CDK Connector Development Kit label Oct 23, 2023

Update airbyte-cdk version in setup

2c9c7b6

tolik0 force-pushed the tolik0/source-s3/wrong-bucket-config-error branch from 0550b77 to 2c9c7b6 Compare October 24, 2023 19:39

Automated Commit - Formatting Changes

fe9d87a

tolik0 merged commit 053d08e into master Oct 24, 2023
17 checks passed

tolik0 deleted the tolik0/source-s3/wrong-bucket-config-error branch October 24, 2023 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Source S3: Add handling NoSuchBucket error #31383

✨ Source S3: Add handling NoSuchBucket error #31383

tolik0 commented Oct 13, 2023 •

edited

Loading

vercel bot commented Oct 13, 2023 •

edited

Loading

github-actions bot commented Oct 13, 2023 •

edited by tolik0

Loading

airbyte-oss-build-runner commented Oct 13, 2023

clnoll Oct 16, 2023

tolik0 Oct 18, 2023

clnoll left a comment

clnoll Oct 18, 2023

tolik0 Oct 19, 2023

clnoll Oct 18, 2023

tolik0 Oct 18, 2023

clnoll Oct 18, 2023

tolik0 Oct 18, 2023

airbyte-oss-build-runner commented Oct 18, 2023

airbyte-oss-build-runner commented Oct 19, 2023

airbyte-oss-build-runner commented Oct 19, 2023

clnoll left a comment

airbyte-oss-build-runner commented Oct 23, 2023

airbyte-oss-build-runner commented Oct 23, 2023

airbyte-oss-build-runner commented Oct 24, 2023

✨ Source S3: Add handling NoSuchBucket error #31383

✨ Source S3: Add handling NoSuchBucket error #31383

Conversation

tolik0 commented Oct 13, 2023 • edited Loading

What

How

vercel bot commented Oct 13, 2023 • edited Loading

github-actions bot commented Oct 13, 2023 • edited by tolik0 Loading

Before Merging a Connector Pull Request

airbyte-oss-build-runner commented Oct 13, 2023

source-s3 test report (commit 75069a5c82) - ❌

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clnoll left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

airbyte-oss-build-runner commented Oct 18, 2023

source-s3 test report (commit c60529b0df) - ❌

airbyte-oss-build-runner commented Oct 19, 2023

source-s3 test report (commit 3c18702032) - ❌

airbyte-oss-build-runner commented Oct 19, 2023

source-s3 test report (commit 30e354ac81) - ❌

clnoll left a comment

Choose a reason for hiding this comment

airbyte-oss-build-runner commented Oct 23, 2023

source-s3 test report (commit 3edc8af363) - ❌

airbyte-oss-build-runner commented Oct 23, 2023

source-s3 test report (commit 0550b77564) - ❌

airbyte-oss-build-runner commented Oct 24, 2023

source-s3 test report (commit fe9d87a2fd) - ✅

tolik0 commented Oct 13, 2023 •

edited

Loading

vercel bot commented Oct 13, 2023 •

edited

Loading

github-actions bot commented Oct 13, 2023 •

edited by tolik0

Loading

source-s3 test report (commit `75069a5c82`) - ❌

source-s3 test report (commit `c60529b0df`) - ❌

source-s3 test report (commit `3c18702032`) - ❌

source-s3 test report (commit `30e354ac81`) - ❌

source-s3 test report (commit `3edc8af363`) - ❌

source-s3 test report (commit `0550b77564`) - ❌

source-s3 test report (commit `fe9d87a2fd`) - ✅