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 - add new reports and bump version #18283

Merged
merged 14 commits into from
Nov 3, 2022
Merged

Source Amazon Seller Partner - add new reports and bump version #18283

merged 14 commits into from
Nov 3, 2022

Conversation

animer3009
Copy link
Contributor

@animer3009 animer3009 commented Oct 21, 2022

What

Added new Amazon Seller Partner Reports

How

with changing Amazon Seller Partner Source connector

Reports Added

GET_FBA_ESTIMATED_FBA_FEES_TXT_DATA
GET_FBA_FULFILLMENT_CURRENT_INVENTORY_DATA
GET_FBA_FULFILLMENT_CUSTOMER_SHIPMENT_PROMOTION_DATA
GET_FBA_FULFILLMENT_INVENTORY_ADJUSTMENTS_DATA
GET_FBA_FULFILLMENT_INVENTORY_RECEIPTS_DATA
GET_FBA_FULFILLMENT_INVENTORY_SUMMARY_DATA
GET_FBA_FULFILLMENT_MONTHLY_INVENTORY_DATA
GET_FBA_MYI_UNSUPPRESSED_INVENTORY_DATA
GET_FBA_SNS_FORECAST_DATA
GET_FBA_SNS_PERFORMANCE_DATA
GET_FLAT_FILE_ARCHIVED_ORDERS_DATA_BY_ORDER_DATE
GET_FLAT_FILE_RETURNS_DATA_BY_RETURN_DATE
GET_MERCHANT_CANCELLED_LISTINGS_DATA
GET_MERCHANT_LISTINGS_DATA
GET_MERCHANT_LISTINGS_DATA_BACK_COMPAT
GET_MERCHANTS_LISTINGS_FYP_REPORT
GET_MERCHANT_LISTINGS_INACTIVE_DATA
GET_STRANDED_INVENTORY_UI_DATA
GET_XML_ALL_ORDERS_DATA_BY_ORDER_DATE_GENERAL
GET_LEDGER_DETAIL_VIEW_DATA
GET_FBA_INVENTORY_PLANNING_DATA
GET_LEDGER_SUMMARY_VIEW_DATA

Copy link
Contributor

@monai monai left a comment

Choose a reason for hiding this comment

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

Thank you for your great overall contribution. I would ask to make some changes and improvements to get it merged.

  1. Add configured catalogs in the integration_tests directory for the newly implemented streams. Don't forget to add them to acceptance-test-config.yml. Uncomment them, run them locally and please share the log.
  2. Update docs/integrations/sources/amazon-seller-partner.md.
  3. Run ./gradlew --no-daemon :airbyte-integrations:connectors:source-amazon-seller-partner:airbytePythonFormat in the root directory.

Check out the documentation on integration tests. It might be helpful.

@sajarin sajarin changed the title add new amazon seller partner reports and bump version Source Amazon Seller Partner - add new reports and bump version Oct 21, 2022
@sajarin sajarin added the bounty-L Maintainer program: claimable large bounty PR label Oct 21, 2022
@github-actions github-actions bot added the area/documentation Improvements or additions to documentation label Oct 21, 2022
@animer3009 animer3009 requested review from monai and removed request for monai October 21, 2022 21:55
@animer3009
Copy link
Contributor Author

@monai
I can't share integrity tests results, because it takes a long time to generate SP reports and possibly Amazon SP will return FATAL as report status. This is causing a failed full test process. So I did it one by one by report type.
So trust it is tested :)

Here is the unit test result.

./gradlew :airbyte-integrations:connectors:source-amazon-seller-partner:unitTest
Screenshot at Oct 21 21-25-14

@vladimir-remar
Copy link
Contributor

Hello @animer3009 thanks for your collaboration I really appreciate it. I've a question, are you take into account the deprecation for this report type GET_FBA_FULFILLMENT_INVENTORY_HEALTH_DATA, here the source. It seems that It will be replaced by a new one GET_FBA_INVENTORY_PLANNING_DATA.
@marcosmarxm Indeed some others will be replaced in the deprecation schedule.

@animer3009
Copy link
Contributor Author

Hello @vladimir-remar,
As GET_FBA_FULFILLMENT_INVENTORY_HEALTH_DATA is still active, I guess we can add it for now. We personally use that report and need to have it for now.
For the future I can add that also, as I am going to add additional reports types too.

Copy link
Contributor

@monai monai left a comment

Choose a reason for hiding this comment

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

After you make the requested changes, run ./gradlew --no-daemon :airbyte-integrations:connectors:source-amazon-seller-partner:airbytePythonFormat in the root directory.

@monai
Copy link
Contributor

monai commented Oct 24, 2022

@monai I can't share integrity tests results, because it takes a long time to generate SP reports and possibly Amazon SP will return FATAL as report status. This is causing a failed full test process. So I did it one by one by report type. So trust it is tested :)

The acceptance test timeout issue can be at least partially mitigated by splitting configured catalog into one stream per catalog. I commented on the relevant code parts. Also, try making start_date in secrets/config.json as recent as possible. It will increase the chances for the tests to fetch actual data and do that quicker than the timeout.

See my comment and the following discussion on that matter: #15843 (comment)

Here is the unit test result.

./gradlew :airbyte-integrations:connectors:source-amazon-seller-partner:unitTest Screenshot at Oct 21 21-25-14

Usually, the acceptance tests cover the addition of new streams. So after you made the requested changes, I would ask you again to run the acceptance tests and post a comment with the entire log as text, not a screenshot.

@animer3009
Copy link
Contributor Author

animer3009 commented Oct 24, 2022

@monai
Test anyway failed a few times.
It is very hard to test it this way. Please have in mind some report types have call frequency limitations..
As I mentioned I did a test using the web interface one by one and it is OK.
I guess we can complete the merge request.

@animer3009 animer3009 requested review from monai and removed request for monai October 24, 2022 22:35
@vladimir-remar
Copy link
Contributor

Hello @vladimir-remar, As GET_FBA_FULFILLMENT_INVENTORY_HEALTH_DATA is still active, I guess we can add it for now. We personally use that report and need to have it for now. For the future I can add that also, as I am going to add additional reports types too.

Thanks for your answer. I will take a look on new ones.

@monai
Copy link
Contributor

monai commented Oct 25, 2022

Test anyway failed a few times.
It is very hard to test it this way. Please have in mind some report types have call frequency limitations..
As I mentioned I did a test using the web interface one by one and it is OK.
I guess we can complete the merge request.

I know well that they often fail, and getting them all to pass in a single run is virtually impossible. Also, I'm aware of severe throttling limits on the Amazon side. So, let me clarify, I'm not asking for a log of all tests passed. On the contrary, I want to see an actual log with a real account to see which tests pass and which fail.

This issue with the long-running and failing tests is not limited to the Amazon SP connector. It's also was the case with Zenloop and Amazon Ads, and I'm mentioning only those I worked with. That's why I asked you to split the configured catalogs, as this change improves the chances for the tests to pass. In addition, such changes and attached logs will help the Airbyte team to find a better way to implement integration tests and enhance the overall quality of all connectors.

So, please run acceptance tests for all the newly added streams and paste the complete log as a text whether they are passing or failing.

Did you update replication_start_date in secrets/config.json? Set it to one month in the past from today, e.g., 2022-09-25T00:00:00; it will be a sweet spot to get enough real data and not too distant to get test generation failed since even the most time-sensitive reports return data up to one month.

And please run ./gradlew format in the root directory. It automatically formats all the code according to the project's code style rules.

@animer3009
Copy link
Contributor Author

@monai

Did you update replication_start_date in secrets/config.json? Set it to one month in the past from today, e.g., 2022-09-25T00:00:00; it will be a sweet spot to get enough real data and not too distant to get test generation failed since even the most time-sensitive reports return data up to one month.

Yep I did.

And please run ./gradlew format in the root directory. It automatically formats all the code according to the project's code style rules.

I guess you mean ./gradlew --no-daemon :airbyte-integrations:connectors:source-amazon-seller-partner:airbytePythonFormat
I do it every time.
It changes nothing.
Here is log attached.
Format Terminal Saved Output.txt

Now I did ./gradlew format.

Also Here are test results:

Integrity Tests Terminal Saved Output.txt

@animer3009
Copy link
Contributor Author

@vladimir-remar
@monai
added additional two report:
GET_FBA_INVENTORY_PLANNING_DATA
and
GET_LEDGER_SUMMARY_VIEW_DATA

@animer3009
Copy link
Contributor Author

@monai Someone else's approval needed to merge?

@monai
Copy link
Contributor

monai commented Oct 31, 2022

@animer3009 Do not push anything to this branch. I'm already publishing it. Instead, open another PR if you have anything to contribute more. See the bot command above #18283 (comment)

And for future contributions, please do not force-push. It ruins commit history and makes commit log hard to track.

@animer3009
Copy link
Contributor Author

@monai yep sorry for that.

@monai
Copy link
Contributor

monai commented Oct 31, 2022

Well. it appears that the force push just broke the publish:

Run git add -u
[amazon_seller_partner_new_reports b92fddd] auto-bump connector version
 2 files changed, 2 insertions(+), 2 deletions(-)
From https://github.com/animer3009/airbyte
 * branch            amazon_seller_partner_new_reports -> FETCH_HEAD
 + f[1](https://github.com/airbytehq/airbyte/actions/runs/3360432785/jobs/5569673218#step:20:1)f7e83b4...6fe04cc50 amazon_seller_partner_new_reports -> origin/amazon_seller_partner_new_reports  (forced update)
fatal: refusing to merge unrelated histories
Error: Process completed with exit code 128.

https://github.com/airbytehq/airbyte/actions/runs/3360432785/jobs/5569673218#step:20:24

@animer3009
Copy link
Contributor Author

@monai can we try republish it?

@animer3009
Copy link
Contributor Author

@monai can I do something?

@monai
Copy link
Contributor

monai commented Oct 31, 2022

The connector image v0.2.28 was built and published successfully. Congratulations, all your implemented streams are ready to use for everyone 🎉

However, I can't merge the branch myself. Someone from the Airbyte team should do that. So please do not push anything to it anymore. Right now, it's in a merge-able state.

@harshithmullapudi can you do that? I also created #18683 that bumps the version in the source definitions list.

@animer3009
Copy link
Contributor Author

@monai I do not see that connector's new version upgrade is available.

@monai
Copy link
Contributor

monai commented Oct 31, 2022

@monai I do not see that connector's new version upgrade is available.

You can upgrade manually for now by entering version 0.2.28. An automatic update check will show that the new version is available when this and #18683 PRs are merged.

@animer3009
Copy link
Contributor Author

@monai Something is wrong? Why it is not merged yet?

@sajarin sajarin merged commit 7693a30 into airbytehq:master Nov 3, 2022
@sajarin
Copy link
Contributor

sajarin commented Nov 3, 2022

Thanks @animer3009 for the PR and @monai for the review.

@animer3009 animer3009 deleted the amazon_seller_partner_new_reports branch December 16, 2022 19:49
@animer3009 animer3009 restored the amazon_seller_partner_new_reports branch December 16, 2022 19:50
@animer3009 animer3009 deleted the amazon_seller_partner_new_reports branch December 16, 2022 19:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

7 participants