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-bing-ads: migrate to per-stream state #17386

Merged
merged 8 commits into from
Sep 29, 2022

Conversation

alafanechere
Copy link
Contributor

What

We want source-bing-ads to use the latest CDK version.

How

  • Fix accountId key types in states (cast int to str)

@github-actions github-actions bot added area/connectors Connector related issues area/documentation Improvements or additions to documentation labels Sep 29, 2022
@alafanechere
Copy link
Contributor Author

alafanechere commented Sep 29, 2022

/test connector=connectors/source-bing-ads

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

Name                          Stmts   Miss  Cover
-------------------------------------------------
source_bing_ads/__init__.py       2      0   100%
source_bing_ads/source.py       259      9    97%
source_bing_ads/cache.py         18      1    94%
source_bing_ads/reports.py      133     17    87%
source_bing_ads/client.py        93     14    85%
-------------------------------------------------
TOTAL                           505     41    92%
	 Name                                                 Stmts   Miss  Cover   Missing
	 ----------------------------------------------------------------------------------
	 source_acceptance_test/base.py                          10      4    60%   15-18
	 source_acceptance_test/config.py                        83      6    93%   78-80, 84-86
	 source_acceptance_test/conftest.py                     164    164     0%   6-282
	 source_acceptance_test/plugin.py                        48     48     0%   6-104
	 source_acceptance_test/tests/test_core.py              329    111    66%   39, 50-58, 63-70, 74-75, 79-80, 164, 202-219, 228-236, 240-245, 251, 284-289, 327-334, 374-376, 379, 439-448, 477-478, 484, 487, 520-530, 543-568, 573-577
	 source_acceptance_test/tests/test_full_refresh.py       52      2    96%   34, 65
	 source_acceptance_test/tests/test_incremental.py       152     26    83%   21-23, 29-31, 36-43, 48-61, 239, 250-258
	 source_acceptance_test/utils/asserts.py                 37      2    95%   57-58
	 source_acceptance_test/utils/common.py                  77     17    78%   15-16, 24-30, 47-54, 64, 67
	 source_acceptance_test/utils/compare.py                 62     23    63%   21-51, 68, 97-99
	 source_acceptance_test/utils/connector_runner.py       112     50    55%   23-26, 32, 36, 39-67, 70-72, 75-77, 80-82, 85-87, 90-92, 95-113, 147-149
	 source_acceptance_test/utils/json_schema_helper.py     105     13    88%   30-31, 38, 41, 65-68, 96, 120, 190-192
	 ----------------------------------------------------------------------------------
	 TOTAL                                                 1358    466    66%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/source_acceptance_test/plugin.py:60: Skipping TestIncremental.test_two_sequential_reads because not found in the config
================== 27 passed, 1 skipped in 537.98s (0:08:57) ===================

@alafanechere alafanechere marked this pull request as ready for review September 29, 2022 15:09
Copy link
Contributor

@brianjlai brianjlai left a comment

Choose a reason for hiding this comment

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

Looks good to me! Nice find on the need to fix the way account was represented in the JSON. It's interesting that this was working in the first place since integers as a key are not considered valid JSON, so somewhere along the way we were translating the int to a str before persisting.

Also attaching a thread for more context on what was being observed during Pedro's OC.
https://airbytehq-team.slack.com/archives/C02TL38U5L7/p1663347369570809

@alafanechere
Copy link
Contributor Author

alafanechere commented Sep 29, 2022

/publish connector=connectors/source-bing-ads run-tests=false

🕑 Publishing the following connectors:
connectors/source-bing-ads
https://github.com/airbytehq/airbyte/actions/runs/3153780291


Connector Did it publish? Were definitions generated?
connectors/source-bing-ads

if you have connectors that successfully published but failed definition generation, follow step 4 here ▶️

@alafanechere alafanechere merged commit 99955ce into master Sep 29, 2022
@alafanechere alafanechere deleted the augustin/migrate-bings-ads-per-stream-state branch September 29, 2022 19:03
jhammarstedt pushed a commit to jhammarstedt/airbyte that referenced this pull request Oct 31, 2022
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 connectors/source/bing-ads
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants