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 Ads: fix portfolioId type #25874

Merged

Conversation

roman-yermilov-gl
Copy link
Collaborator

What

Fix portfolioId type to integer (Docs)

@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/amazon-ads labels May 8, 2023
@github-actions
Copy link
Contributor

github-actions bot commented May 8, 2023

Before Merging a Connector Pull Request

Wow! What a great pull request you have here! 🎉

To merge this PR, ensure the following has been done/considered for each connector added or updated:

  • PR name follows PR naming conventions
  • Breaking changes are considered. If a Breaking Change is being introduced, ensure an Airbyte engineer has created a Breaking Change Plan and you've followed all steps in the Breaking Changes Checklist
  • Connector version has been incremented in the Dockerfile and metadata.yaml according to our Semantic Versioning for Connectors guidelines
  • Secrets in the connector's spec are annotated with airbyte_secret
  • All documentation files are up to date. (README.md, bootstrap.md, docs.md, etc...)
  • Changelog updated in docs/integrations/<source or destination>/<name>.md with an entry for the new version. See changelog example
  • You, or an Airbyter, have run /test successfully on this PR - or on a non-forked branch
  • You, or an Airbyter, have run /publish successfully on this PR - or on a non-forked branch
  • You've updated the connector's metadata.yaml file (new!)
  • The Octavia bot updated the source_definitions.yaml or destination_definitions.yaml, or you ran processResources manually (deprecated)

If the checklist is complete, but the CI check is failing,

  1. Check for hidden checklists in your PR description

  2. Toggle the github label checklist-action-run on/off to re-run the checklist CI.

@roman-yermilov-gl
Copy link
Collaborator Author

roman-yermilov-gl commented May 8, 2023

/test connector=connectors/source-amazon-ads

🕑 connectors/source-amazon-ads https://github.com/airbytehq/airbyte/actions/runs/4914389887
❌ connectors/source-amazon-ads https://github.com/airbytehq/airbyte/actions/runs/4914389887
🐛

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: can't populate stream because it requires real ad campaign
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:587: Backward compatibility tests are disabled for version 1.0.4.
=================== 35 passed, 3 skipped in 71.53s (0:01:11) ===================

@roman-yermilov-gl
Copy link
Collaborator Author

roman-yermilov-gl commented May 9, 2023

/test connector=connectors/source-amazon-ads

🕑 connectors/source-amazon-ads https://github.com/airbytehq/airbyte/actions/runs/4925401787
✅ connectors/source-amazon-ads https://github.com/airbytehq/airbyte/actions/runs/4925401787
Python tests coverage:

Name                                                              Stmts   Miss  Cover
-------------------------------------------------------------------------------------
source_amazon_ads/utils.py                                           22      0   100%
source_amazon_ads/streams/sponsored_products.py                      41      0   100%
source_amazon_ads/streams/sponsored_display.py                       31      0   100%
source_amazon_ads/streams/sponsored_brands.py                        26      0   100%
source_amazon_ads/streams/report_streams/products_report.py          37      0   100%
source_amazon_ads/streams/report_streams/display_report.py           18      0   100%
source_amazon_ads/streams/report_streams/brands_video_report.py      12      0   100%
source_amazon_ads/streams/report_streams/brands_report.py            12      0   100%
source_amazon_ads/streams/report_streams/__init__.py                  5      0   100%
source_amazon_ads/streams/profiles.py                                21      0   100%
source_amazon_ads/streams/attribution_report.py                      81      0   100%
source_amazon_ads/streams/__init__.py                                 7      0   100%
source_amazon_ads/schemas/sponsored_products.py                      37      0   100%
source_amazon_ads/schemas/sponsored_display.py                       31      0   100%
source_amazon_ads/schemas/sponsored_brands.py                        22      0   100%
source_amazon_ads/schemas/profile.py                                 16      0   100%
source_amazon_ads/schemas/attribution_report.py                      21      0   100%
source_amazon_ads/schemas/__init__.py                                 7      0   100%
source_amazon_ads/constants.py                                        1      0   100%
source_amazon_ads/__init__.py                                         2      0   100%
source_amazon_ads/streams/common.py                                  79      1    99%
source_amazon_ads/schemas/common.py                                  50      1    98%
source_amazon_ads/source.py                                          48      1    98%
source_amazon_ads/streams/report_streams/report_streams.py          255     20    92%
-------------------------------------------------------------------------------------
TOTAL                                                               882     23    97%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: can't populate stream because it requires real ad campaign
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:587: Backward compatibility tests are disabled for version 1.0.4.
=================== 35 passed, 3 skipped in 76.45s (0:01:16) ===================

@erohmensing erohmensing requested review from alafanechere and removed request for erohmensing May 10, 2023 21:50
@alafanechere alafanechere added the breaking-change Don't merge me unless you are ready. label May 11, 2023
Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

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

@roman-yermilov-gl this type change is a breaking change. I'm going to create a breaking change release plan that I will share in this PR. Could you please link the original issue that led you to implement this change? Is there any functional implication in keeping these fields as decimal today? If it prevents you from deserializing the API response I would suggest to use a type transformer. If this is leading to unusable data on the destination side this change is indeed required.

docs/integrations/sources/amazon-ads.md Outdated Show resolved Hide resolved
@alafanechere
Copy link
Contributor

@roman-yermilov-gl here's the breaking change release plan draft. I'd need more context on why this change is introduced to move forward.

Copy link
Contributor

@alafanechere alafanechere left a comment

Choose a reason for hiding this comment

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

@roman-yermilov-gl do you confirm the modified streams are present in the configured catalog we use for acceptance tests? Could you please update the expected records file with data corresponding to the modified streams?

@roman-yermilov-gl
Copy link
Collaborator Author

@roman-yermilov-gl this type change is a breaking change. I'm going to create a breaking change release plan that I will share in this PR. Could you please link the original issue that led you to implement this change? Is there any functional implication in keeping these fields as decimal today? If it prevents you from deserializing the API response I would suggest to use a type transformer. If this is leading to unusable data on the destination side this change is indeed required.

This is an issue #17623

@roman-yermilov-gl
Copy link
Collaborator Author

@roman-yermilov-gl do you confirm the modified streams are present in the configured catalog we use for acceptance tests? Could you please update the expected records file with data corresponding to the modified streams?

No. We don't have data for sponsored streams and we cannot populate those streams with data as they require real ad campaign

@alafanechere
Copy link
Contributor

@roman-yermilov-gl thank you for the details. I updated the breaking change document. I suspect the error mentioned #17623 to only impact BigQuery denormalized. As we have no test data it's hard to assess the breadth of this problem. I suggest to not merge it until we either:

  • Have more evidence that this bug is widespread and impacting multiple users
  • Having a painless release strategy for breaking change

@roman-yermilov-gl
Copy link
Collaborator Author

@roman-yermilov-gl thank you for the details. I updated the breaking change document. I suspect the error mentioned #17623 to only impact BigQuery denormalized. As we have no test data it's hard to assess the breadth of this problem. I suggest to not merge it until we either:

  • Have more evidence that this bug is widespread and impacting multiple users
  • Having a painless release strategy for breaking change

I agree but what would be your suggestion for next steps? Should I put the task #17623 on hold?

@alafanechere alafanechere self-assigned this May 16, 2023
@alafanechere
Copy link
Contributor

I agree but what would be your suggestion for next steps? Should I put the task #17623 on hold?

Yes I think so. I'll ask on the issue to try to seed the account with seed data.

@roman-yermilov-gl
Copy link
Collaborator Author

I agree but what would be your suggestion for next steps? Should I put the task #17623 on hold?

Yes I think so. I'll ask on the issue to try to seed the account with seed data.

Hi @alafanechere
Is there updates on this isssue?

@alafanechere
Copy link
Contributor

@roman-yermilov-gl I got not update from the stakeholders on #17623
Now that we can feature flag connectors on cloud I encourage you to take ownership on this doc and suggest a rollout plan in coordination with TCS.

@roman-yermilov-gl roman-yermilov-gl force-pushed the ryermilov/source-amazon-ads-fix-portfolioId-type branch from 54775ab to 2313868 Compare May 31, 2023 13:39
@roman-yermilov-gl
Copy link
Collaborator Author

roman-yermilov-gl commented May 31, 2023

/test connector=connectors/source-amazon-ads

🕑 connectors/source-amazon-ads https://github.com/airbytehq/airbyte/actions/runs/5133648775
✅ connectors/source-amazon-ads https://github.com/airbytehq/airbyte/actions/runs/5133648775
Python tests coverage:

Name                                                              Stmts   Miss  Cover
-------------------------------------------------------------------------------------
source_amazon_ads/utils.py                                           22      0   100%
source_amazon_ads/streams/sponsored_products.py                      41      0   100%
source_amazon_ads/streams/sponsored_display.py                       31      0   100%
source_amazon_ads/streams/sponsored_brands.py                        26      0   100%
source_amazon_ads/streams/report_streams/products_report.py          37      0   100%
source_amazon_ads/streams/report_streams/display_report.py           21      0   100%
source_amazon_ads/streams/report_streams/brands_video_report.py      12      0   100%
source_amazon_ads/streams/report_streams/brands_report.py            12      0   100%
source_amazon_ads/streams/report_streams/__init__.py                  5      0   100%
source_amazon_ads/streams/profiles.py                                21      0   100%
source_amazon_ads/streams/__init__.py                                 7      0   100%
source_amazon_ads/schemas/sponsored_products.py                      40      0   100%
source_amazon_ads/schemas/sponsored_display.py                       34      0   100%
source_amazon_ads/schemas/sponsored_brands.py                        22      0   100%
source_amazon_ads/schemas/profile.py                                 16      0   100%
source_amazon_ads/schemas/attribution_report.py                      21      0   100%
source_amazon_ads/schemas/__init__.py                                 7      0   100%
source_amazon_ads/constants.py                                        1      0   100%
source_amazon_ads/__init__.py                                         2      0   100%
source_amazon_ads/streams/common.py                                  79      1    99%
source_amazon_ads/schemas/common.py                                  53      1    98%
source_amazon_ads/source.py                                          48      1    98%
source_amazon_ads/streams/attribution_report.py                      82      6    93%
source_amazon_ads/streams/report_streams/report_streams.py          255     20    92%
-------------------------------------------------------------------------------------
TOTAL                                                               895     29    97%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: can't populate stream because it requires real ad campaign
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:603: Backward compatibility tests are disabled for version 1.1.0.
======================== 36 passed, 3 skipped in 58.56s ========================

@roman-yermilov-gl
Copy link
Collaborator Author

roman-yermilov-gl commented Jun 12, 2023

/test connector=connectors/source-amazon-ads

🕑 connectors/source-amazon-ads https://github.com/airbytehq/airbyte/actions/runs/5242150601
✅ connectors/source-amazon-ads https://github.com/airbytehq/airbyte/actions/runs/5242150601
Python tests coverage:

Name                                                              Stmts   Miss  Cover
-------------------------------------------------------------------------------------
source_amazon_ads/utils.py                                           22      0   100%
source_amazon_ads/streams/sponsored_products.py                      41      0   100%
source_amazon_ads/streams/sponsored_display.py                       31      0   100%
source_amazon_ads/streams/sponsored_brands.py                        26      0   100%
source_amazon_ads/streams/report_streams/products_report.py          37      0   100%
source_amazon_ads/streams/report_streams/display_report.py           21      0   100%
source_amazon_ads/streams/report_streams/brands_video_report.py      12      0   100%
source_amazon_ads/streams/report_streams/brands_report.py            12      0   100%
source_amazon_ads/streams/report_streams/__init__.py                  5      0   100%
source_amazon_ads/streams/profiles.py                                21      0   100%
source_amazon_ads/streams/__init__.py                                 7      0   100%
source_amazon_ads/schemas/sponsored_products.py                      40      0   100%
source_amazon_ads/schemas/sponsored_display.py                       34      0   100%
source_amazon_ads/schemas/sponsored_brands.py                        22      0   100%
source_amazon_ads/schemas/profile.py                                 16      0   100%
source_amazon_ads/schemas/attribution_report.py                      21      0   100%
source_amazon_ads/schemas/__init__.py                                 7      0   100%
source_amazon_ads/constants.py                                        1      0   100%
source_amazon_ads/__init__.py                                         2      0   100%
source_amazon_ads/streams/common.py                                  79      1    99%
source_amazon_ads/schemas/common.py                                  53      1    98%
source_amazon_ads/source.py                                          48      1    98%
source_amazon_ads/streams/attribution_report.py                      82      6    93%
source_amazon_ads/streams/report_streams/report_streams.py          255     20    92%
-------------------------------------------------------------------------------------
TOTAL                                                               895     29    97%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/plugin.py:63: Skipping TestIncremental.test_two_sequential_reads: can't populate stream because it requires real ad campaign
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:607: Backward compatibility tests are disabled for version 1.1.0.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:695: This tests currently leads to too much failures. We need to fix the connectors at scale first.
=================== 35 passed, 4 skipped in 60.23s (0:01:00) ===================

@roman-yermilov-gl
Copy link
Collaborator Author

roman-yermilov-gl commented Jun 12, 2023

/publish connector=connectors/source-amazon-ads pre-release=true run-tests=false

⚠️ The publish slash command is now deprecated.

The connector publication happens on merge to the master branch.

Please use /legacy-publish if you need to publish normalization images.

Please join the #publish-on-merge-updates slack channel to track ongoing publish pipelines.

Please reach out to the @dev-connector-ops team if you need support in publishing a connector.

@alafanechere
Copy link
Contributor

Updating the branch to get a fresh CI run.

@roman-yermilov-gl roman-yermilov-gl enabled auto-merge (squash) June 20, 2023 15:33
@octavia-squidington-iii
Copy link
Collaborator

source-amazon-ads test report (commit 9c41aeb7e6) - ✅

⏲️ Total pipeline duration: 194 seconds

Step Result
Validate airbyte-integrations/connectors/source-amazon-ads/metadata.yaml
Connector version semver check.
Connector version increment check.
QA checks
Code format checks
Connector package install
Build source-amazon-ads docker image for platform linux/x86_64
Unit tests
Acceptance tests

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-amazon-ads test

@octavia-squidington-iii
Copy link
Collaborator

source-amazon-ads test report (commit 221cdc4104) - ✅

⏲️ Total pipeline duration: 211 seconds

Step Result
Validate airbyte-integrations/connectors/source-amazon-ads/metadata.yaml
Connector version semver check.
Connector version increment check.
QA checks
Code format checks
Connector package install
Build source-amazon-ads docker image for platform linux/x86_64
Unit tests
Acceptance tests

🔗 View the logs here

Please note that tests are only run on PR ready for review. Please set your PR to draft mode to not flood the CI engine and upstream service on following commits.
You can run the same pipeline locally on this branch with the airbyte-ci tool with the following command

airbyte-ci connectors --name=source-amazon-ads test

@roman-yermilov-gl roman-yermilov-gl merged commit 80e6a26 into master Jun 20, 2023
23 checks passed
@roman-yermilov-gl roman-yermilov-gl deleted the ryermilov/source-amazon-ads-fix-portfolioId-type branch June 20, 2023 16:22
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 breaking-change Don't merge me unless you are ready. checklist-action-run connectors/source/amazon-ads
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Source Amazon Ads: schema typed incorrectly, "Could not evaluate union" exception
5 participants