-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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: add Redirect URI & Tenant ID to spec #11311
🎉 Source Bing Ads: add Redirect URI & Tenant ID to spec #11311
Conversation
738491f
to
2fad16b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the contribution! I'm going to run the acceptance test. Could you please define the version as 0.1.4
rather than bumping to 0.2.0
? Could you also please bump the version in airbyte-config/init/src/main/resources/seed/source_definitions.yaml
?
/test connector=connectors/source-bing-ads
|
@alafanechere have you added the new fields to your test config.json? |
2df6f62
to
9b01ecc
Compare
No, I did not but as the field are not required the current config should work 🤔 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alexbondar92 I locally ran the acceptance with a config that has the fields you added and it worked. Without these fields the following error occurs:
{"type": "LOG", "log": {"level": "FATAL", "message": "__init__() missing 2 required positional arguments: 'tenant_id' and 'redirect_uri'\nTraceback (most recent call last):\n File \"/airbyte/integration_code/main.py\", line 13, in <module>\n launch(source, sys.argv[1:])\n File \"/usr/local/lib/python3.7/site-packages/airbyte_cdk/entrypoint.py\", line 127, in launch\n for message in source_entrypoint.run(parsed_args):\n File \"/usr/local/lib/python3.7/site-packages/airbyte_cdk/entrypoint.py\", line 112, in run\n catalog = self.source.discover(self.logger, config)\n File \"/usr/local/lib/python3.7/site-packages/airbyte_cdk/sources/abstract_source.py\", line 72, in discover\n streams = [stream.as_airbyte_stream() for stream in self.streams(config=config)]\n File \"/airbyte/integration_code/source_bing_ads/source.py\", line 598, in streams\n client = Client(**config)\nTypeError: __init__() missing 2 required positional arguments: 'tenant_id' and 'redirect_uri'"}}
Requesting a review from @misteryeo as the |
airbyte-integrations/connectors/source-bing-ads/source_bing_ads/client.py
Show resolved
Hide resolved
/test connector=connectors/source-bing-ads
|
Hi @alexbondar92 I updated our CI secrets according to your update. Could you please pull and merge master into your branch and push again? There are some changes on master that might fix the test run 🙏 |
@alafanechere done |
@alexbondar92 Could you please pull and merge master again. Sorry to ask again but some new changes in master should now make the test run 🙏 |
@alafanechere done :) |
/test connector=connectors/source-bing-ads
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I confirm acceptance tests are passing when I add your two new fields to our test config.
/publish connector=connectors/source-bing-ads run-tests=false
|
Thanks @alexbondar92 for the contribution and your patience in the review process! |
@alafanechere Quick reminder that the image for this needs to be pushed before merging. It looks like it was only tested here and not published. |
Wait it looks like publish was run but the image isn't available on Docker hub? |
OK, this is strange. I can pull the image with |
I can pull it too! |
/publish connector=connectors/source-bing-ads
|
@alexbondar92 can you clarify something in this PR: redirect_uri doesn't seem to be used anywhere in the code. See #12488 for the context. Can you clarify how using this param is required for your use case? |
What
Microsoft doesn't allow to use common endpoint in new applications.
error on setup connector:
Currently bing ads connector sends ""(empty str) at redirect uri during authentication(
OAuthWebAuthCodeGrant
).How
Add optional Redirect URI & Tenant ID fields to spec, and send it during client's authentication.
For back compatibility and for use of applications before
10/15/2018
, default is ""(empty str) andcommon
like before.Recommended reading order
spec.json
client.py
Pre-merge Checklist
Expand the relevant checklist and delete the others.
Updating a connector
Community member or Airbyter
airbyte_secret
./gradlew :airbyte-integrations:connectors:<name>:integrationTest
.README.md
bootstrap.md
. See description and examplesdocs/integrations/<source or destination>/<name>.md
including changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>
command is passing/publish
command described hereTests
Unit
Test session starts (platform: darwin, Python 3.9.7, pytest 6.2.5, pytest-sugar 0.9.4)
cachedir: .pytest_cache
rootdir: /Users/alexb/Connecteam/airbyte-ct, configfile: pytest.ini
plugins: sugar-0.9.4, timeout-1.4.2
collecting ... {"type": "LOG", "log": {"level": "DEBUG", "message": "opening (file:////Users/alexb/Connecteam/airbyte-ct/airbyte-integrations/connectors/source-bing-ads/.venv/lib/python3.9/site-packages/bingads/v13/proxies/sandbox/campaignmanagement_service.xml)"}}
airbyte-integrations/connectors/source-bing-ads/unit_tests/test_client.py::test_sudsobject_todict_primitive_types ✓ 8% ▉
airbyte-integrations/connectors/source-bing-ads/unit_tests/test_client.py::test_sudsobject_todict_nested ✓ 17% █▋
airbyte-integrations/connectors/source-bing-ads/unit_tests/test_client.py::test_is_expired_true ✓ 25% ██▌
airbyte-integrations/connectors/source-bing-ads/unit_tests/test_client.py::test_is_expired_true_with_delta_threshold ✓ 33% ███▍
airbyte-integrations/connectors/source-bing-ads/unit_tests/test_client.py::test_is_expired_false ✓ 42% ████▎
airbyte-integrations/connectors/source-bing-ads/unit_tests/test_reports.py::test_get_column_value ✓ 50% █████
airbyte-integrations/connectors/source-bing-ads/unit_tests/test_reports.py::test_get_updated_state_new_state ✓ 58% █████▉
airbyte-integrations/connectors/source-bing-ads/unit_tests/test_reports.py::test_get_updated_state_state_uncahanged ✓ 67% ██████▋
airbyte-integrations/connectors/source-bing-ads/unit_tests/test_reports.py::test_get_updated_state_state_new_account ✓ 75% ███████▌
airbyte-integrations/connectors/source-bing-ads/unit_tests/test_reports.py::test_get_report_record_timestamp_daily ✓ 83% ████████▍
airbyte-integrations/connectors/source-bing-ads/unit_tests/test_reports.py::test_get_report_record_timestamp_without_aggregation ✓ 92% █████████▎
airbyte-integrations/connectors/source-bing-ads/unit_tests/test_reports.py::test_get_report_record_timestamp_hourly ✓ 100% ██████████
=========================================================================================== warnings summary ============================================================================================
unit_tests/test_reports.py:17
/Users/alexb/Connecteam/airbyte-ct/airbyte-integrations/connectors/source-bing-ads/unit_tests/test_reports.py:17: PytestCollectionWarning: cannot collect test class 'TestReport' because it has a init constructor (from: airbyte-integrations/connectors/source-bing-ads/unit_tests/test_reports.py)
class TestReport(ReportsMixin, SourceBingAds):
-- Docs: https://docs.pytest.org/en/stable/warnings.html
Results (4.68s):
12 passed
Integration
Put your integration tests output here.
Test session starts (platform: darwin, Python 3.9.7, pytest 6.2.5, pytest-sugar 0.9.4)
cachedir: .pytest_cache
rootdir: /Users/alexb/Connecteam/airbyte-ct, configfile: pytest.ini
plugins: sugar-0.9.4, timeout-1.4.2
collecting ...
Results (0.05s):
Acceptance