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 Amazon Seller Partner: Error for max wait time reached when create report status is CANCELLED or FATAL #4108

Merged
merged 2 commits into from
Jun 16, 2021

Conversation

harshithmullapudi
Copy link
Contributor

@harshithmullapudi harshithmullapudi commented Jun 14, 2021

What

Error for max wait time reached when create report status is CANCELLED or FATAL

How

When create report status returns either of the status we pass to next month

2. Fix: Error for max wait time reached when create report status is CANCELLED or FATAL
@marcosmarxm marcosmarxm added CDK Connector Development Kit community_pr area/connectors Connector related issues and removed CDK Connector Development Kit labels Jun 14, 2021
@@ -66,21 +66,6 @@ def test_check_connection(mocker):

assert ORDERS_RESPONSE == base_client.check_connection()


def test_get_cursor_state():
Copy link
Contributor

Choose a reason for hiding this comment

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

why did you get rid of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed this function also not needed anymore

@@ -8,6 +8,10 @@
"title": "Orders",
"description": "All orders that were updated after a specified date",
"properties": {
"seller_id": {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a new field? or was this field previously ignored?

"type": "string",
"description": "The role’s arn (needs permission to “Assume Role” STS)"
},
"seller_id": {
Copy link
Contributor

Choose a reason for hiding this comment

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

what is this used for?

Copy link
Contributor

Choose a reason for hiding this comment

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

spoke offline with Harshith: This is used to generate the refresh_token and is how amazon actually know what data to return in the query even though this id is not passed into the query at all. Because of this, it's actually useful to inject this data into the generated record so users know where the data came from.

Copy link
Contributor

@davinchia davinchia left a comment

Choose a reason for hiding this comment

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

Nice! The status changes makes sense to me. Couple of questions so I understand what the seller id is used for - if it's not part of the api, why pass it in?

  1. Can you run ./gradlew format to make sure formatting is correct?
  2. Can you add a local test with mocks to make sure the status logic is correct.
  3. Can you add a changelog to the directory similar to https://github.com/airbytehq/airbyte/blob/master/airbyte-integrations/connectors/source-shopify/CHANGELOG.md? Not all our sources have this, but we are trying to get there.
  4. Can you update the amazon seller partner documentation with the additional parameter being added?

@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Jun 15, 2021
@marcosmarxm marcosmarxm changed the title Error for max wait time reached when create report status is CANCELLED or FATAL Source Amazon Seller Partner: Error for max wait time reached when create report status is CANCELLED or FATAL Jun 16, 2021
@harshithmullapudi
Copy link
Contributor Author

Can we merge this or is there something pending from my side @tuliren ?

@tuliren
Copy link
Contributor

tuliren commented Jun 16, 2021

There are two more versions that need to be bumped. But I can do that for you. Will merge the PR first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants