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.

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

🐛 Source Shopify: fix None cursor field value, access_scopes keyError issue, JSONDecodeError issue, mark datefields as format of date #25110

Merged
merged 21 commits into from Apr 24, 2023

Conversation

bazarnov
Copy link
Collaborator

@bazarnov bazarnov commented Apr 12, 2023

What

Resolves:

How

  • fixed default_state_comparison when the state has an explicit value of None, while get_updated_state()
  • added user-friendly error message for cases when access_scopes could not be verified, because of bad credentials
  • added handling for 403, when the stream lacks the permissions (both while AvailabilityCheck and regular sync)
  • added missing properties for certain JSON Schemas to satisfy the CAT requirements.
  • migrated to new format for CAT and enabled high strictness

🚨 User Impact 🚨

This PR introduces non-breaking changes for stream schemas. Users likely to update their schemas should reset the data and re-sync once again for selected streams.

Pre-merge Checklist

Expand the relevant checklist and delete the others.

Updating a connector

Community member or Airbyter

  • Unit & integration tests added and passing. Community members, please provide proof of success locally e.g: screenshot or copy-paste unit, integration, and 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.

  • Code reviews completed

  • Connector version has been incremented

  • Documentation updated

    • Connector's README.md
    • Connector's bootstrap.md. See description and examples
    • Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
  • PR name follows PR naming conventions

Airbyter

If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.

  • Create a non-forked branch based on this PR and test the below items on it
  • Build is successful
  • If new credentials are required for use in CI, add them to GSM. Instructions.
  • /test connector=connectors/<name> command is passing
  • New Connector version released on Dockerhub and connector version bumped by running the /publish command described here

@octavia-squidington-iii octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Apr 12, 2023
@bazarnov
Copy link
Collaborator Author

bazarnov commented Apr 12, 2023

/test connector=connectors/source-shopify

🕑 connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/4678464705

@bazarnov bazarnov changed the title 🐛 Source Shopify: fix None cursor field value, access_scopes keyError issues 🐛 Source Shopify: fix None cursor field value, access_scopes keyError issue, JSONDecodeError issue, mark datefields as format of date Apr 14, 2023
@bazarnov bazarnov linked an issue Apr 14, 2023 that may be closed by this pull request
@bazarnov
Copy link
Collaborator Author

bazarnov commented Apr 14, 2023

/test connector=connectors/source-shopify

🕑 connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/4700975252
✅ connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/4700975252
Python tests coverage:

Name                               Stmts   Miss  Cover
------------------------------------------------------
source_shopify/shopify_schema.py   10152      0   100%
source_shopify/auth.py                20      0   100%
source_shopify/__init__.py             2      0   100%
source_shopify/transform.py           58      3    95%
source_shopify/utils.py               96      7    93%
source_shopify/source.py             452     66    85%
source_shopify/graphql.py             47     40    15%
------------------------------------------------------
TOTAL                              10827    116    99%

Build Passed

Test summary info:

All Passed

@bazarnov
Copy link
Collaborator Author

bazarnov commented Apr 15, 2023

/test connector=connectors/source-shopify

🕑 connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/4706697759
✅ connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/4706697759
Python tests coverage:

Name                               Stmts   Miss  Cover
------------------------------------------------------
source_shopify/shopify_schema.py   10152      0   100%
source_shopify/auth.py                20      0   100%
source_shopify/__init__.py             2      0   100%
source_shopify/transform.py           58      3    95%
source_shopify/utils.py               96      7    93%
source_shopify/source.py             452     66    85%
source_shopify/graphql.py             47     40    15%
------------------------------------------------------
TOTAL                              10827    116    99%

Build Passed

Test summary info:

All Passed

@bazarnov bazarnov requested a review from a team April 15, 2023 08:55
@bazarnov
Copy link
Collaborator Author

bazarnov commented Apr 18, 2023

/test connector=connectors/source-shopify

🕑 connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/4730685291
✅ connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/4730685291
Python tests coverage:

Name                               Stmts   Miss  Cover
------------------------------------------------------
source_shopify/shopify_schema.py   10152      0   100%
source_shopify/auth.py                20      0   100%
source_shopify/__init__.py             2      0   100%
source_shopify/transform.py           58      3    95%
source_shopify/utils.py               96      7    93%
source_shopify/source.py             452     66    85%
source_shopify/graphql.py             47     40    15%
------------------------------------------------------
TOTAL                              10827    116    99%

Build Passed

Test summary info:

All Passed

Copy link
Collaborator

@lazebnyi lazebnyi left a comment

Choose a reason for hiding this comment

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

Nice!

@lazebnyi lazebnyi requested a review from clnoll April 18, 2023 16:03
@bazarnov
Copy link
Collaborator Author

bazarnov commented Apr 19, 2023

/test connector=connectors/source-shopify

🕑 connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/4742187886
✅ connectors/source-shopify https://github.com/airbytehq/airbyte/actions/runs/4742187886
Python tests coverage:

Name                               Stmts   Miss  Cover
------------------------------------------------------
source_shopify/shopify_schema.py   10152      0   100%
source_shopify/auth.py                20      0   100%
source_shopify/__init__.py             2      0   100%
source_shopify/transform.py           58      3    95%
source_shopify/utils.py               96      7    93%
source_shopify/source.py             454     66    85%
source_shopify/graphql.py             47     40    15%
------------------------------------------------------
TOTAL                              10829    116    99%

Build Passed

Test summary info:

All Passed

@bazarnov bazarnov requested review from bnchrch and removed request for davydov-d, artem1205 and clnoll April 19, 2023 11:59
Copy link
Contributor

@bnchrch bnchrch left a comment

Choose a reason for hiding this comment

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

Approving with a Big caveat below.

Looking at our Connector Breaking Change Release Playbook it seems like this is a non-breaking change. But it "feels" like it close to it.

@bazarnov Can you confirm this meets none of this criteria below and if we released no user connections would be broken requiring action on their end.

If so then feel free to merge, if not please hold off.

Breaking changes

  • Spec Change - The configuration required by users of this connector have been changed and syncs will fail until users reconfigure or re-authenticate. This change is not possible via a Config Migration
  • Schema Change - The type of a property previously present within a record has changed
  • Stream or Property Removal - Data that was previously being synced is no longer going to be synced.
  • Destination Format / Normalization Change - The way the destination writes the final data or how normalization cleans that data is changing in a way that requires a full-refresh.
  • State Changes - The format of the source’s state has changed, and the full dataset will need to be re-synced

@bazarnov
Copy link
Collaborator Author

@bnchrch
This PR introduces non-breaking changes for stream schemas (we add new, not remove existing properties)

  1. Users likely to update their schemas should reset the data and re-sync once again for selected streams.
  2. Adding the format to spec doesn't affect the current users, it implies the DatePicker in UI mainly.
  3. The schema data types remain the same.
  4. All other improvements don't affect current users.

No impact is expected. Tested locally the transition from 0.3.2 > 0.3.3 (this changes), works as expected + normalization has no issues.

Publishing this now and merging it. Thanks for the review!

@bazarnov
Copy link
Collaborator Author

bazarnov commented Apr 20, 2023

/publish connector=connectors/source-shopify

🕑 Publishing the following connectors:
connectors/source-shopify
https://github.com/airbytehq/airbyte/actions/runs/4753412551


Connector Version Did it publish? Were definitions generated?
connectors/source-shopify 0.3.3

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@bazarnov
Copy link
Collaborator Author

bazarnov commented Apr 24, 2023

/publish connector=connectors/source-shopify

🕑 Publishing the following connectors:
connectors/source-shopify
https://github.com/airbytehq/airbyte/actions/runs/4789768849


Connector Version Did it publish? Were definitions generated?
connectors/source-shopify 0.3.3

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@bazarnov bazarnov merged commit 26c9ad5 into master Apr 24, 2023
27 checks passed
@bazarnov bazarnov deleted the baz/source-shopify-1099-1021 branch April 24, 2023 20:47
marcosmarxm pushed a commit to natalia-miinto/airbyte that referenced this pull request Jun 8, 2023
…rror issue, `JSONDecodeError` issue, mark `datefields` as format of `date` (airbytehq#25110)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/shopify
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source Shopify: Mark datefields in properly as dates
4 participants