feat(source-bing-ads): migrate accounts stream to manifest by aldogonzalez8 · Pull Request #59136 · airbytehq/airbyte

Aldo Gonzalez (aldogonzalez8) · 2025-04-28T23:38:06Z

What

Fix the unit and integration tests to pass as the base source was changed to YamlDeclarativeSource

Depends on: airbytehq/airbyte-python-cdk#519
Resolves: https://github.com/airbytehq/airbyte-internal-issues/issues/12668

How

User helper for source creation.

Review guide

User Impact

A step further to migrate to low code.

Can this PR be safely reverted and rolled back?

YES 💚
NO ❌

vercel · 2025-04-28T23:38:11Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
airbyte-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 6, 2025 2:46pm

Aldo Gonzalez (aldogonzalez8) · 2025-04-29T15:26:12Z

...ions/connectors/source-bing-ads/unit_tests/integrations/test_app_install_ad_labels_stream.py

        output, _ = self.read_stream(self.stream_name, SyncMode.full_refresh, self._config, "app_install_ad_labels_empty")
        assert len(output.records) == 0
-        assert len(output.logs) == 11
+        assert len(output.logs) == 12


The ConcurrentDeclarativeSource in read() calls self. _ group_streams (), which calls self.streams()

At the end of that, it calls super.read(), which is AbstractSource.read(), which collects streams again.

The override of the streams method in source-bing-ads creates an authentication client, so we initialize it twice, and we receive an extra log entry for "Fetching access token ...".

Should we just remove this? It does not seem to indicate any behavior for this connector and is going to be flaky if we add logs in the CDK

Can we check in the history is there is a reason it was added?

I removed it but added support to check the behavior, meaning I assert the expected logs for the test behavior.

Maxime Carbonneau-Leclerc (maxi297) · 2025-04-29T16:10:10Z

airbyte-integrations/connectors/source-bing-ads/unit_tests/integrations/client_builder.py

    )
+
+
+def build_request_2(config: Dict[str, Any]) -> HttpRequest:


I assume this is temporary until we migrate all to REST, right? Can we add a document why this is different? I guess this is just the order or stuff

Yes, I have documented this.

Maxime Carbonneau-Leclerc (maxi297)

Can we remove the user stream here?
Do we have difference between REST and SOAP for account stream?

Maxime Carbonneau-Leclerc (maxi297) · 2025-04-29T16:11:45Z

airbyte-integrations/connectors/source-bing-ads/unit_tests/integrations/base_test.py

            ) as service_call_mock:
                catalog = CatalogBuilder().with_stream(stream_name, sync_mode).build()
-                return read(SourceBingAds(), config, catalog, state, expecting_exception), service_call_mock
+                # return read(SourceBingAds(), config, catalog, state, expecting_exception), service_call_mock


nit: to remove

Copy-paste is the source of all evil, removed.

Maxime Carbonneau-Leclerc (maxi297) · 2025-04-29T16:12:16Z

airbyte-integrations/connectors/source-bing-ads/unit_tests/integrations/request_builder.py

+
+class RequestBuilder:
+    def __init__(self, resource: str = None) -> None:
+        self._spreadsheet_id = None


nit: clean this

Copy-paste is the source of all evil, removed.

Maxime Carbonneau-Leclerc (maxi297) · 2025-04-29T16:14:46Z

...ions/connectors/source-bing-ads/unit_tests/integrations/test_app_install_ad_labels_stream.py

        output, _ = self.read_stream(self.stream_name, SyncMode.full_refresh, self._config, "app_install_ad_labels_empty")
        assert len(output.records) == 0
-        assert len(output.logs) == 11
+        assert len(output.logs) == 12


Should we just remove this? It does not seem to indicate any behavior for this connector and is going to be flaky if we add logs in the CDK

Can we check in the history is there is a reason it was added?

….logs

Aldo Gonzalez (aldogonzalez8) · 2025-05-02T15:10:32Z

Ok, interpretation of these regressions_tests results:

test_catalog_are_the_same[CONNECTION 9ee7920] [failed]
- is_file_based appeared, this is expected in cdk ^6.45.10
- Many "number" attributes were changed "integer" so this was expected as well.
TestDataIntegrity.test_record_schema_match_with_state[CONNECTION 9ee7920] [failed]
- TaxInformation is returned in rest version of the API if empty ("[]"), this was not happening for soap but I don't think this is a problem, just as follow up will try to find a connection with "TaxInformation" not empty.
TestDataIntegrity.test_all_records_are_the_same_with_state[CONNECTION 9ee7920] [failed]
- Same thing, TaxInformation is there even if there is no data to show "[]"

Follo up: find a connection with TaxInformation

Aldo Gonzalez (aldogonzalez8) · 2025-05-06T00:23:22Z

Follow up: find a connection with TaxInformation

Sadly, I couldn't find any connection with TaxInformation filled to compare, so I think I'm okay with letting it be as it is, making the RC, and seeing if we get any feedback from this change.

What I did was create a test for the LinkedAgencies transformation we just added.

I ran another Progressive Rollout for a connection with four accounts and got similar results to exposed above.

LinkedAgencies: We get it even if empty array, as we have a transformation to iterate to make it compatible with SOAP version.
TaxInformation: for REST it is returned as an empty array when no data, this is not happening for SOAP but as this is additive I think this is not a problem.

I expect to create the RC tomorrow and follow up.

cc Brian Lai (@brianjlai)

maxi297 and others added 10 commits April 22, 2025 14:01

upgrade cdk version

1ee9be9

update release information

2599e47

fix release information

a9c0e6c

adding pendulum as it was removed from transitive dep

5c16ec4

[TMP] move accounts to lowcode

57cdf31

fix schema, pagination and tenant

5241bc0

remove fixme

7f9ec6f

mlc: initial changes to make unit tests pass

b687270

mlc: make most of unit test pass with source helpers

cf70e38

mlc: ruff format

ea05817

Aldo Gonzalez (aldogonzalez8) self-assigned this Apr 28, 2025

Octavia Squidington III (octavia-squidington-iii) added the connectors/source/bing-ads label Apr 28, 2025

vercel bot deployed to Preview April 28, 2025 23:48 View deployment

Aldo Gonzalez (aldogonzalez8) added 2 commits April 28, 2025 19:00

mlc: add unit test for accounts stream

d52ea4c

mlc: ruff format

e173919

vercel bot deployed to Preview April 29, 2025 02:08 View deployment

Aldo Gonzalez (aldogonzalez8) mentioned this pull request Apr 29, 2025

feat(cdk): allow empty secrets (bing-ads) airbytehq/airbyte-python-cdk#519

Merged

poetry lock

cba9304

vercel bot deployed to Preview April 29, 2025 14:46 View deployment

Aldo Gonzalez (aldogonzalez8) commented Apr 29, 2025

View reviewed changes

Maxime Carbonneau-Leclerc (maxi297) reviewed Apr 29, 2025

View reviewed changes

Maxime Carbonneau-Leclerc (maxi297) approved these changes Apr 29, 2025

View reviewed changes

Aldo Gonzalez (aldogonzalez8) changed the base branch from maxi297/migrate-accounts-to-low-code to master April 29, 2025 17:42

Aldo Gonzalez (aldogonzalez8) marked this pull request as ready for review April 29, 2025 17:42

remove unnecesary imports and patches

a43c384

vercel bot deployed to Preview April 29, 2025 17:51 View deployment

cleanup and documment

f6de83e

vercel bot deployed to Preview April 29, 2025 18:15 View deployment

change count of logs with checking specific empty data logs in output…

9d31c83

….logs

vercel bot deployed to Preview April 29, 2025 19:41 View deployment

remove comment of accounts stream

3239dbc

vercel bot deployed to Preview April 30, 2025 00:04 View deployment

Aldo Gonzalez (aldogonzalez8) added 3 commits April 29, 2025 17:25

move to inline schema

701c209

ruff format

8d5f109

Merge branch 'master' into ac8/migrate-accounts-to-low-code

0657d09

vercel bot deployed to Preview April 30, 2025 14:38 View deployment

Aldo Gonzalez (aldogonzalez8) added 2 commits April 30, 2025 14:21

comment out disabled secret

d91ddb0

comment out disabled secret

ad2d0f8

vercel bot deployed to Preview April 30, 2025 21:50 View deployment

Aldo Gonzalez (aldogonzalez8) added 2 commits April 30, 2025 16:49

add transformation to keep previous schema

e5eb1e5

prettier yaml

7cf746b

vercel bot deployed to Preview April 30, 2025 23:58 View deployment

bump cdk version

43149b1

vercel bot deployed to Preview May 1, 2025 14:55 View deployment

Merge branch 'master' into ac8/migrate-accounts-to-low-code

10c153b

vercel bot deployed to Preview May 1, 2025 19:54 View deployment

add transformations for id and date

bf029a8

vercel bot deployed to Preview May 1, 2025 23:51 View deployment

move accounts schema to schemas section

16243e3

vercel bot deployed to Preview May 2, 2025 00:08 View deployment

Aldo Gonzalez (aldogonzalez8) added 2 commits May 5, 2025 18:13

add test for linked agencies

4339879

ruff format

8bb767b

vercel bot deployed to Preview May 6, 2025 00:21 View deployment

update release information

1faccba

vercel bot deployed to Preview May 6, 2025 14:46 View deployment

Aldo Gonzalez (aldogonzalez8) merged commit 32c5638 into master May 6, 2025
27 checks passed

Aldo Gonzalez (aldogonzalez8) deleted the ac8/migrate-accounts-to-low-code branch May 6, 2025 15:38

		)


		def build_request_2(config: Dict[str, Any]) -> HttpRequest:

Conversation

Aldo Gonzalez (aldogonzalez8) commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

Review guide

User Impact

Can this PR be safely reverted and rolled back?

Uh oh!

vercel bot commented Apr 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Maxime Carbonneau-Leclerc (maxi297) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aldo Gonzalez (aldogonzalez8) commented May 2, 2025

Uh oh!

Aldo Gonzalez (aldogonzalez8) commented May 6, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Aldo Gonzalez (aldogonzalez8) commented Apr 28, 2025 •

edited

Loading

vercel bot commented Apr 28, 2025 •

edited

Loading