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.

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

fix for Source Amazon Seller Partner: only 100 orders are read #9115

Closed
wants to merge 11 commits into from
Closed

fix for Source Amazon Seller Partner: only 100 orders are read #9115

wants to merge 11 commits into from

Conversation

prudhvi85
Copy link
Contributor

@prudhvi85 prudhvi85 commented Dec 26, 2021 •
edited
Loading

What

Amazon seller partner reads only 100 records and terminates. (issue #9001 )

How

Next token is not in the root level of steam_data, it is nested in payload. Marketplace ids should be always part of the request.

🚨 User Impact 🚨

No breaking changes

@CLAassistant
Copy link

CLAassistant commented Dec 26, 2021 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added the area/connectors Connector related issues label Dec 26, 2021
@harshithmullapudi
Copy link
Contributor

harshithmullapudi commented Dec 27, 2021 •
edited
Loading

Hey @prudhvi85 can you sign Agreement?

harshithmullapudi
harshithmullapudi suggested changes Dec 27, 2021
View reviewed changes
Copy link
Contributor

@harshithmullapudi harshithmullapudi left a comment

Choose a reason for hiding this comment

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

https://docs.airbyte.io/connector-development#4.-publish-the-connector can you check these steps here

...integrations/connectors/source-amazon-seller-partner/source_amazon_seller_partner/streams.py Outdated
@@ -551,8 +551,8 @@ def request_params(
self, stream_state: Mapping[str, Any], next_page_token: Mapping[str, Any] = None, **kwargs
) -> MutableMapping[str, Any]:
params = super().request_params(stream_state=stream_state, next_page_token=next_page_token, **kwargs)
if not next_page_token:
params.update({"MarketplaceIds": ",".join(self.marketplace_ids)})
#if not next_page_token:
Copy link
Contributor

Choose a reason for hiding this comment

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

can we remove this line?

Copy link
Contributor Author

@prudhvi85 prudhvi85 Dec 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Removed the commented if condition

@prudhvi85
Copy link
Contributor Author

Hey @prudhvi85 can you sign Agreement?

Hey @harshithmullapudi I did sign it. and rechecked it more than 10 times. When I try to redo it again here is what it looks like

image

@htrueman htrueman linked an issue Dec 27, 2021 that may be closed by this pull request
🐛 Source Amazon Seller Partner: only 100 orders are read. #9001
Closed
@htrueman htrueman mentioned this pull request Dec 27, 2021
Closed
@alafanechere alafanechere mentioned this pull request Dec 27, 2021
Closed
@alafanechere alafanechere temporarily deployed to more-secrets December 27, 2021 17:52 Inactive
@jrhizor jrhizor temporarily deployed to more-secrets December 27, 2021 17:53 Inactive
@alafanechere
Copy link
Contributor

alafanechere commented Dec 27, 2021 •
edited
Loading

Hi @prudhvi85, to make the CLA signature work, please make sure the commits you made are with the same email as the one you're using for GitHub. You committed with your agility.com address, and the screenshot you attached shows a Gmail address, so maybe your GitHub account is also registered with your Gmail address; this makes the CLA signature fails.
If you're not eventually succeeding, I can still create a PR myself from your branch and merge, but it will be a pity because you won't be recorded as the contributor for this fix 😢 .

We also need you to bump the connector version in the following file:

  • airbyte-integrations/connectors/source-amazon-seller-partner/Dockerfile
  • airbyte-config/init/src/main/resources/seed/source_definitions.yaml
  • airbyte-config/init/src/main/resources/config/STANDARD_SOURCE_DEFINITION/e55879a8-0ef8-4557-abcf-ab34c53ec460.json
  • +Update Changelog : docs/integrations/sources/amazon-seller-partner.md

@prudhvi85
Copy link
Contributor Author

Thank you for the suggeston. Signed the CLA successfully

@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Dec 27, 2021
@prudhvi85
Copy link
Contributor Author

@harshithmullapudi can you please check the changes and approve the pull request

@harshithmullapudi
Copy link
Contributor

Hey @prudhvi85 it looks good but I can see that there is something messed in the md file can you fix it?
https://github.com/prudhvi85/airbyte/blob/sp_api_fix/docs/integrations/sources/amazon-seller-partner.md

@harshithmullapudi
Copy link
Contributor

Can you also share screenshot for unit tests and integration tests ?

@prudhvi85
Copy link
Contributor Author

@harshithmullapudi can you please check the changes and approve the pull request

sure, I think it is due to my editor.

@alafanechere
Copy link
Contributor

alafanechere commented Jan 3, 2022 •
edited
Loading

@prudhvi85 and @harshithmullapudi I merged another contributor PR #9212 with version 0.2.9, please merge master, fix conflicts and bump the version again 🙏 .

@prudhvi85
Copy link
Contributor Author

prudhvi85 commented Jan 4, 2022 •
edited
Loading

@alafanechere can you please merge to master
Bumped version to 0.2.10.

@prudhvi85
Copy link
Contributor Author

Can you also share screenshot for unit tests and integration tests ?

image

image

@harshithmullapudi
Copy link
Contributor

LGTM @prudhvi85 I will publish it sometime in the evening today.

@harshithmullapudi
Copy link
Contributor

Hey, @prudhvi85 also need some help to understand when you say The connector is not working.

@prudhvi85
Copy link
Contributor Author

today
@harshithmullapudi my bad there was a merge issue in my code. I fixed it. it's working fine now

@harshithmullapudi harshithmullapudi mentioned this pull request Jan 4, 2022
Merged
@harshithmullapudi
Copy link
Contributor

Hey @prudhvi85 I have pulled your commits into #9294 as there was one more PR which got merged. I did this to avoid more back and forth and merge this quick. Closing this.

@prudhvi85 prudhvi85 deleted the sp_api_fix branch January 5, 2022 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@avida avida avida approved these changes

@harshithmullapudi harshithmullapudi Awaiting requested review from harshithmullapudi

Assignees

@harshithmullapudi harshithmullapudi

Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation community connectors/source/amazon-seller-partner connectors/sources-api
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

🐛 Source Amazon Seller Partner: only 100 orders are read.
9 participants
@prudhvi85 @CLAassistant @harshithmullapudi @alafanechere @avida @jrhizor @igrankova @octavia-squidington-iii @prudhviagility