Skip to content

feat(shopify)!: Sync all orders not just open orders #11528

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
Merged
kodiakhq merged 2 commits into from
Jun 15, 2023
Merged

feat(shopify)!: Sync all orders not just open orders #11528

kodiakhq merged 2 commits into from
Jun 15, 2023

Conversation

@bbernays
Copy link
Collaborator

@bbernays bbernays commented Jun 13, 2023 •
edited
Loading

Summary

Closes #11529

BEGIN_COMMIT_OVERRIDE
feat(shopify)!: Sync all orders not just open orders (#11528)
BREAKING-CHANGE: Sync all orders rather than just open orders. This can cause sync times to drastically increase as there are more records to sync.
END_COMMIT_OVERRIDE

By default the orders api only returns open orders. This PR changes the behavior to return all orders

https://shopify.dev/docs/api/admin-rest/2023-04/resources/order#get-orders?status=any

@bbernays bbernays requested review from a team and amanenk and removed request for a team June 13, 2023 22:27
@cq-bot cq-bot added the shopify label Jun 13, 2023
@bbernays bbernays linked an issue Jun 13, 2023 that may be closed by this pull request
feat: Shopify Orders should sync historical data #11529
Closed
1 task
@bbernays bbernays changed the title feat(shopify): Fetch all orders not just open orders feat(shopify)!: Sync all orders not just open orders Jun 13, 2023
@bbernays bbernays requested a review from disq June 13, 2023 22:35
@erezrokah erezrokah added the automerge Automatically merge once required checks pass label Jun 14, 2023
@candiduslynx
Copy link
Contributor

candiduslynx commented Jun 14, 2023 •
edited
Loading

I've removed the automerge label for a moment.

Could you clarify why is it marked as breaking?

cc: @bbernays @erezrokah

@candiduslynx candiduslynx removed the automerge Automatically merge once required checks pass label Jun 14, 2023
@disq
Copy link
Member

disq commented Jun 14, 2023

Testing in a big shop with multiple years of orders, will report back.

@disq
Copy link
Member

disq commented Jun 14, 2023 •
edited
Loading

60k orders in the last 40 minutes, still going (min(created_at) shows Jan 2022 so far). Too much? Maybe table_options later? Could also have two separate tables - shopify_orders and shopify_all_orders, to enable skipping?

The Shopify resource suggests:

Only the last 60 days' worth of orders from a store are accessible from the Order resource by default. If you want to access older orders, then you need to request access to all orders. If your app is granted access, then you can add the read_all_orders scope to your app along with read_orders or write_orders.

But looks like I'm able to fetch all orders (the api key I'm using from my app doesn't have read_all_orders scope - checked)

update - I cancelled after fetching 73k orders in 48 minutes to preserve API limits.

@bbernays
Copy link
Collaborator Author

bbernays commented Jun 14, 2023

@candiduslynx - It is marked as a major change because it can completely break users flow now that it is grabbing so much data

@candiduslynx
Copy link
Contributor

candiduslynx commented Jun 14, 2023

I think we might want to reconsider/split this data, so the users may skip it, or add an option at least

@bbernays
Copy link
Collaborator Author

bbernays commented Jun 14, 2023

I think we might want to reconsider/split this data, so the users may skip it, or add an option at least

There are a lot of different parameters that we could add. Adding them in a coherent manner is more than minor task. I would suggest we release it as is and if we recieve feedback that users need more control we can expose overrides

@bbernays bbernays mentioned this pull request Jun 14, 2023
Open
1 task
@candiduslynx
Copy link
Contributor

candiduslynx commented Jun 15, 2023

@bbernays @erezrokah I'm fine with releasing this as is, but could I ask to have the commit override to tell people what huge impact this change does have on the sync time?

@bbernays
Copy link
Collaborator Author

bbernays commented Jun 15, 2023

I'm fine with releasing this as is, but could I ask to have the commit override to tell people what huge impact this change does have on the sync time?

@candiduslynx Done

@candiduslynx candiduslynx added the automerge Automatically merge once required checks pass label Jun 15, 2023
@kodiakhq kodiakhq bot merged commit e647334 into cloudquery:main Jun 15, 2023
@cq-bot cq-bot mentioned this pull request Jun 15, 2023
Merged
@bbernays bbernays deleted the all-orders branch June 15, 2023 19:36
kodiakhq bot pushed a commit that referenced this pull request Jul 14, 2023
@cq-bot
chore(main): Release plugins-source-shopify v3.0.0 (#11601)
7ca730b 
🤖 I have created a release *beep* *boop*
---


## [3.0.0](plugins-source-shopify-v2.0.2...plugins-source-shopify-v3.0.0) (2023-07-14)


### ⚠ BREAKING CHANGES

* Upgrades the shopify source plugin to use plugin-sdk v4. This version does not contain any user-facing breaking changes, but because it is now using CloudQuery gRPC protocol v3, it does require use of a destination plugin that also supports protocol v3. All recent destination plugin versions support this.
* **shopify:** Sync all orders rather than just open orders. This can cause sync times to drastically increase as there are more records to sync.

### Features

* **shopify:** Sync all orders not just open orders (#11528) ([e647334](e647334))
* Upgrades the shopify source plugin to use plugin-sdk v4. This version does not contain any user-facing breaking changes, but because it is now using CloudQuery gRPC protocol v3, it does require use of a destination plugin that also supports protocol v3. All recent destination plugin versions support this. ([f7f838a](f7f838a))


### Bug Fixes

* **deps:** Update github.com/apache/arrow/go/v13 digest to 5a06b2e ([#11857](#11857)) ([43c2f5f](43c2f5f))
* **deps:** Update github.com/cloudquery/arrow/go/v13 digest to 0656028 ([#11739](#11739)) ([7a6ad49](7a6ad49))
* **deps:** Update github.com/cloudquery/arrow/go/v13 digest to 0a52533 ([#12091](#12091)) ([927cefa](927cefa))
* **deps:** Update github.com/cloudquery/arrow/go/v13 digest to 1e68c51 ([#11637](#11637)) ([46043bc](46043bc))
* **deps:** Update github.com/cloudquery/arrow/go/v13 digest to 43638cb ([#11672](#11672)) ([3c60bbb](3c60bbb))
* **deps:** Update github.com/cloudquery/arrow/go/v13 digest to 4d76231 ([#11532](#11532)) ([6f04233](6f04233))
* **deps:** Update github.com/cloudquery/arrow/go/v13 digest to 8366a22 ([#11717](#11717)) ([8eeff5b](8eeff5b))
* **deps:** Update github.com/cloudquery/arrow/go/v13 digest to 95d3199 ([#11708](#11708)) ([03f214f](03f214f))
* **deps:** Update github.com/cloudquery/arrow/go/v13 digest to a2a76eb ([#12104](#12104)) ([311f474](311f474))
* **deps:** Update github.com/cloudquery/arrow/go/v13 digest to b0832be ([#11651](#11651)) ([71e8c29](71e8c29))
* **deps:** Update github.com/cloudquery/arrow/go/v13 digest to d864719 ([#11611](#11611)) ([557a290](557a290))
* **deps:** Update github.com/cloudquery/arrow/go/v13 digest to df3b664 ([#11882](#11882)) ([9635b22](9635b22))
* **deps:** Update github.com/cloudquery/arrow/go/v13 digest to f060192 ([#11730](#11730)) ([c7019c2](c7019c2))
* **deps:** Update github.com/cloudquery/arrow/go/v13 digest to f0dffc6 ([#11689](#11689)) ([18ac0e9](18ac0e9))
* **deps:** Update module github.com/cloudquery/plugin-pb-go to v1.1.0 ([#11665](#11665)) ([d8947c9](d8947c9))
* **deps:** Update module github.com/cloudquery/plugin-pb-go to v1.2.0 ([#11720](#11720)) ([7ef521d](7ef521d))
* **deps:** Update module github.com/cloudquery/plugin-pb-go to v1.2.1 ([#11722](#11722)) ([309be72](309be72))
* **deps:** Update module github.com/cloudquery/plugin-pb-go to v1.3.3 ([#11726](#11726)) ([f0ca611](f0ca611))
* **deps:** Update module github.com/cloudquery/plugin-pb-go to v1.3.4 ([#11753](#11753)) ([cd4fe1c](cd4fe1c))
* **deps:** Update module github.com/cloudquery/plugin-pb-go to v1.5.0 ([#11850](#11850)) ([3255857](3255857))
* **deps:** Update module github.com/cloudquery/plugin-pb-go to v1.6.0 ([#11916](#11916)) ([421e752](421e752))
* **deps:** Update module github.com/cloudquery/plugin-pb-go to v1.7.0 ([#12166](#12166)) ([94390dd](94390dd))
* **deps:** Update module github.com/cloudquery/plugin-sdk/v3 to v3.10.6 ([#11473](#11473)) ([7272133](7272133))
* **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.1.0 ([#12174](#12174)) ([80f0289](80f0289))
* **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.1.1 ([#12185](#12185)) ([cfaff16](cfaff16))
* **deps:** Upgrade source plugins to SDK v4.0.0 release ([#12135](#12135)) ([c20a111](c20a111))
* Don't try to Close backend connection when it's `nil` ([#12124](#12124)) ([9faf370](9faf370))
* Update SDK on sources ([#11983](#11983)) ([0da0bcf](0da0bcf))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erezrokah erezrokah erezrokah approved these changes

@amanenk amanenk Awaiting requested review from amanenk

@disq disq Awaiting requested review from disq

Assignees

No one assigned

Labels

automerge Automatically merge once required checks pass

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

feat: Shopify Orders should sync historical data

5 participants

@bbernays @candiduslynx @disq @erezrokah @cq-bot