Source S3: don't require history to be present to identify legacy state format #29520

clnoll · 2023-08-17T14:38:28Z

history data is not required for the state message to be valid, so we should handle the case when it's not present.

…te format

github-actions · 2023-08-17T14:38:49Z

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.

octavia-squidington-iii · 2023-08-17T14:43:54Z

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

⏲️ Total pipeline duration: 03mn20s

Step	Result
Validate airbyte-integrations/connectors/source-s3/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	❌
QA checks	✅
Code format checks	✅
Connector package install	✅
Build source-s3 docker image for platform linux/x86_64	✅
Unit tests	❌

🔗 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

girarda

what's the scenario leading to a state message without a history?

clnoll · 2023-08-17T18:03:49Z

@girarda this came up while testing CATs; I'm not totally sure there's a real-world scenario where we'd expect to hit it.

maxi297

LGTM! Can we merge this into #29471?

girarda · 2023-08-18T12:36:47Z

I see from this thread that this was because of the abnormal state CAT test. I think the abnormal state is bad in the sense it should have a history, but I'm also fine handling it this way in the connector. Either way, this shouldn't happen in a real life situation. 🚢 !

maxi297 · 2023-08-18T13:36:51Z

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

@@ -68,15 +68,17 @@ def _is_legacy_state(value: StreamState) -> bool:
            return False
        try:
            # Verify datetime format in history
-            item = list(value.get("history", {}).keys())[0]
-            datetime.strptime(item, Cursor._DATE_FORMAT)
+            history = value.get("history", {}).keys()


Individually, _is_legacy_state is fine now but set_initial_state will raise ValueError because _convert_legacy_state will have an empty converted_history

Synced on this - this case shouldn't happen but we will handle it if it does.

github-actions · 2023-08-18T14:28:19Z

source-s3 test report (commit `25d7624a1d`) - ❌

⏲️ Total pipeline duration: 13mn08s

Step	Result
Validate airbyte-integrations/connectors/source-s3/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	✅
QA checks	✅
Code format checks	❌
Connector package install	✅
Build source-s3 docker image for platform linux/x86_64	✅
Unit tests	✅
Integration tests	✅
Acceptance tests	✅

🔗 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

github-actions · 2023-08-18T15:08:39Z

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

⏲️ Total pipeline duration: 12mn42s

Step	Result
Validate airbyte-integrations/connectors/source-s3/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	✅
QA checks	✅
Code format checks	❌
Connector package install	✅
Build source-s3 docker image for platform linux/x86_64	✅
Unit tests	✅
Integration tests	✅
Acceptance tests	✅

🔗 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

github-actions · 2023-08-18T17:35:02Z

source-s3 test report (commit `4148832e14`) - ✅

⏲️ Total pipeline duration: 13mn31s

Step	Result
Validate airbyte-integrations/connectors/source-s3/metadata.yaml	✅
Connector version semver check	✅
Connector version increment check	✅
QA checks	✅
Code format checks	✅
Connector package install	✅
Build source-s3 docker image for platform linux/x86_64	✅
Unit tests	✅
Integration tests	✅
Acceptance tests	✅

🔗 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

sentry-io · 2023-08-25T04:47:29Z

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

‼️ ValueError: The config options you selected are no longer supported. /airbyte/integration_code/source_s3/v4/legacy_c... View Issue
‼️ ValueError: Malformed advanced_options config json: Expecting property name enclosed in double quotes: line 1... /airbyte/integration_code/source_s3/v4/legacy_c... View Issue

_{Did you find this useful? React with a 👍 or 👎}

…te format (airbytehq#29520)

Source S3: don't require history to be present to identify legacy sta…

cfee106

…te format

clnoll requested a review from maxi297 August 17, 2023 14:38

octavia-squidington-iii added area/connectors Connector related issues connectors/source/s3 labels Aug 17, 2023

clnoll requested a review from girarda August 17, 2023 16:17

girarda reviewed Aug 17, 2023

View reviewed changes

girarda approved these changes Aug 17, 2023

View reviewed changes

clnoll added 2 commits August 17, 2023 14:24

fix test

d042aec

version bump

6512b0a

maxi297 approved these changes Aug 18, 2023

View reviewed changes

maxi297 reviewed Aug 18, 2023

View reviewed changes

Handle empty history

25d7624

octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Aug 18, 2023

Source s3 error on unsupported config (#29541)

cfdc274

clnoll enabled auto-merge (squash) August 18, 2023 15:05

formatting

4148832

clnoll merged commit 620a941 into master Aug 18, 2023
20 of 22 checks passed

clnoll deleted the fix-legacy-state-identification branch August 18, 2023 17:35

harrytou pushed a commit to KYVENetwork/airbyte that referenced this pull request Sep 1, 2023

Source S3: don't require history to be present to identify legacy sta…

d448248

…te format (airbytehq#29520)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Source S3: don't require history to be present to identify legacy state format #29520

Source S3: don't require history to be present to identify legacy state format #29520

clnoll commented Aug 17, 2023

github-actions bot commented Aug 17, 2023

octavia-squidington-iii commented Aug 17, 2023

girarda left a comment

clnoll commented Aug 17, 2023

maxi297 left a comment

girarda commented Aug 18, 2023

maxi297 Aug 18, 2023

clnoll Aug 18, 2023

github-actions bot commented Aug 18, 2023

github-actions bot commented Aug 18, 2023

github-actions bot commented Aug 18, 2023

sentry-io bot commented Aug 25, 2023 •

edited

Source S3: don't require history to be present to identify legacy state format #29520

Source S3: don't require history to be present to identify legacy state format #29520

Conversation

clnoll commented Aug 17, 2023

github-actions bot commented Aug 17, 2023

Before Merging a Connector Pull Request

octavia-squidington-iii commented Aug 17, 2023

source-s3 test report (commit cfee106a41) - ❌

girarda left a comment

Choose a reason for hiding this comment

clnoll commented Aug 17, 2023

maxi297 left a comment

Choose a reason for hiding this comment

girarda commented Aug 18, 2023

maxi297 Aug 18, 2023

Choose a reason for hiding this comment

clnoll Aug 18, 2023

Choose a reason for hiding this comment

github-actions bot commented Aug 18, 2023

source-s3 test report (commit 25d7624a1d) - ❌

github-actions bot commented Aug 18, 2023

source-s3 test report (commit cfdc274d0c) - ❌

github-actions bot commented Aug 18, 2023

source-s3 test report (commit 4148832e14) - ✅

sentry-io bot commented Aug 25, 2023 • edited

Suspect Issues

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

source-s3 test report (commit `25d7624a1d`) - ❌

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

source-s3 test report (commit `4148832e14`) - ✅

sentry-io bot commented Aug 25, 2023 •

edited