🐛 Source Amazon Seller Partner: add `app_id` to the OAuth Flow (source `spec` side) #27110

bazarnov · 2023-06-07T11:24:51Z

What

Resolving: #25773

How

edited spec.py by adding app_id to the advanced_auth specification to get this parameter from instance-wide params provided by Airbyte
removed pydantic spec implementation and use the spec.json instead for easy modifications.
corrected test_source.py unit tests
updated public docs due to removed app_id

🚨 User Impact 🚨

This change is breaking since the spec has changed.

github-actions · 2023-06-07T11:25:12Z

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've updated the connector's metadata.yaml file (new!)

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

Check for hidden checklists in your PR description
Toggle the github label checklist-action-run on/off to re-run the checklist CI.

bazarnov · 2023-06-07T11:26:17Z

airbyte-integrations/connectors/source-amazon-seller-partner/metadata.yaml

  dockerRepository: airbyte/source-amazon-seller-partner
  githubIssueLabel: source-amazon-seller-partner
  icon: amazonsellerpartner.svg
  license: MIT
  name: Amazon Seller Partner
  registries:
    cloud:
-      enabled: false
+      enabled: true


🔴 This is set intentionally for the dev-* env OAuth tests !!! 🔴

This param will be set to True only when we decide to release the source back to the Cloud after all the tests are passed and OAuth is working again.

Have we completed all the tests and can we leave the flag as true?

how are you testing this on the dev envs? If doing so via connector version overrides in LaunchDarkly this shouldn't need to be set to true for this case.

Yes, the results of the tests are available in secondary PR: https://github.com/airbytehq/airbyte-platform-internal/pull/7085

…eller-partner-oauth-fix

bazarnov · 2023-06-08T21:03:40Z

Pre-released version 1.2.1-dev.d38d6fba12 is published successfully.

https://github.com/airbytehq/airbyte/actions/runs/5215664388

bazarnov · 2023-06-09T09:36:59Z

The new spec without app_id for 1.2.1-dev.d38d6fba12 image tag has been tested here: https://github.com/airbytehq/airbyte-platform-internal/pull/7085

The release version would have the tag 1.3.0 because we introduce breaking changes here.

We can move on and merge this in, but I've faced other issues while check_connection. It turns out that Role ARN should also be supplied + AWS access/secret keys. Otherwise, we receive many meaningless config errors, even though we've passed the OAuth Flow correctly it's still required.

@marcosmarxm why would these params be optional for the source-amazon-seller-partner considering this PR: #23300 In my point of view, they should be required.

…eller-partner-oauth-fix

bazarnov · 2023-06-09T14:06:38Z

Changed GSM configs start_date to 2022-09-01 to guarantee Orders stream data presence.

bazarnov · 2023-06-10T19:01:15Z

/test connector=connectors/source-amazon-seller-partner

🕑 connectors/source-amazon-seller-partner https://github.com/airbytehq/airbyte/actions/runs/5231481118
✅ connectors/source-amazon-seller-partner https://github.com/airbytehq/airbyte/actions/runs/5231481118
Python tests coverage:

Name                                        Stmts   Miss  Cover
---------------------------------------------------------------
source_amazon_seller_partner/__init__.py        2      0   100%
source_amazon_seller_partner/constants.py      38      1    97%
source_amazon_seller_partner/source.py         42     11    74%
source_amazon_seller_partner/streams.py       583    242    58%
source_amazon_seller_partner/auth.py           61     36    41%
---------------------------------------------------------------
TOTAL                                         726    290    60%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:109: Backward compatibility tests are disabled for version 1.2.0.
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.2.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.
================== 38 passed, 3 skipped in 2377.67s (0:39:37) ==================

lazebnyi · 2023-06-12T11:29:29Z

airbyte-integrations/connectors/source-amazon-seller-partner/metadata.yaml

  dockerRepository: airbyte/source-amazon-seller-partner
  githubIssueLabel: source-amazon-seller-partner
  icon: amazonsellerpartner.svg
  license: MIT
  name: Amazon Seller Partner
  registries:
    cloud:
-      enabled: false
+      enabled: true


Have we completed all the tests and can we leave the flag as true?

...-integrations/connectors/source-amazon-seller-partner/source_amazon_seller_partner/source.py

...yte-integrations/connectors/source-amazon-seller-partner/integration_tests/sample_state.json

…eller-partner-oauth-fix

bazarnov · 2023-06-12T12:37:26Z

/test connector=connectors/source-amazon-seller-partner

🕑 connectors/source-amazon-seller-partner https://github.com/airbytehq/airbyte/actions/runs/5243639822
✅ connectors/source-amazon-seller-partner https://github.com/airbytehq/airbyte/actions/runs/5243639822
Python tests coverage:

Name                                        Stmts   Miss  Cover
---------------------------------------------------------------
source_amazon_seller_partner/__init__.py        2      0   100%
source_amazon_seller_partner/constants.py      38      1    97%
source_amazon_seller_partner/source.py         43     11    74%
source_amazon_seller_partner/streams.py       583    242    58%
source_amazon_seller_partner/auth.py           61     36    41%
---------------------------------------------------------------
TOTAL                                         727    290    60%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:109: Backward compatibility tests are disabled for version 1.2.0.
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.2.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.
================== 38 passed, 3 skipped in 2663.70s (0:44:23) ==================

lazebnyi

LGTM!

pedroslopez

Could you clarify why this is a breaking change? Is the change just removing a previously-required field? If so, it should be fine if this field is passed through an old config - am i missing something?

pedroslopez · 2023-06-12T19:06:53Z

airbyte-integrations/connectors/source-amazon-seller-partner/metadata.yaml

  dockerRepository: airbyte/source-amazon-seller-partner
  githubIssueLabel: source-amazon-seller-partner
  icon: amazonsellerpartner.svg
  license: MIT
  name: Amazon Seller Partner
  registries:
    cloud:
-      enabled: false
+      enabled: true


how are you testing this on the dev envs? If doing so via connector version overrides in LaunchDarkly this shouldn't need to be set to true for this case.

pedroslopez · 2023-06-12T19:07:35Z

...ons/connectors/source-amazon-seller-partner/source_amazon_seller_partner/schemas/Orders.json

are these schema changes related to the spec change?

how are you testing this on the dev envs? If doing so via connector version overrides in LaunchDarkly this shouldn't need to be set to true for this case.

I've deployed to dev-2 and added the pre-release version mentioned above for the tests using +New Connector directly.

I guess the cloud.enabled: true was not necessary, but I thought the process would be easier before the deployment took place, as missed the fact that the connector was hidden from the cloud.

I didn't use Launch Darkly. I've made changes directly on the dev-2 db with connectors' source-definition-id and was able to create and test the OAuth Flow for the pre-release custom connector.

are these schema changes related to the spec change?

No. This eventually appeared after the Orders stream has finally enabled for the tests. It was not tested by CAT before.

I didn't use Launch Darkly. I've made changes directly on the dev-2 db with connectors' source-definition-id and was able to create and test the OAuth Flow for the pre-release custom connector.

Ah got it- this worked because the connector is not on the cloud catalog. As a note, if something is in the catalog any manual changes made in the DB will get overwritten 30 seconds later by an automatic update that occurs.

Apparently, not for oauth_configs db part)

bazarnov · 2023-06-12T20:42:22Z

Could you clarify why this is a breaking change? Is the change just removing a previously-required field? If so, it should be fine if this field is passed through an old config - am i missing something?

The field app_id is completely removed as the non-necessary field, it's just not used elsewhere but for OAuth purposes, which we do for the user with this change.

pedroslopez · 2023-06-12T21:04:53Z

The field app_id is completely removed as the non-necessary field, it's just not used elsewhere but for OAuth purposes, which we do for the user with this change.

Got it - this change in itself shouldn't be breaking. However, I'd like to understand what happens to users that have already authorized:

Do we have any users on cloud using this connector that have authorized with a manually provided app id?
If so, what happens to existing oauth authentications? would they stay connected given that we might be passing a different app_id?
Was the app_id only used for oauth? If not, does this have any effect on OSS users that would have passed app_id in their config?

bazarnov · 2023-06-12T21:13:26Z

Do we have any users on cloud using this connector that have authorized with a manually provided app id?

As far as this issue says - we do not: https://github.com/airbytehq/oncall/issues/2017#issuecomment-1563064033

If so, what happens to existing oauth authentications? would they stay connected given that we might be passing a different app_id?

Based on the answer in point 1. No. It was not working for 3rd-parties before, and half-working for those who tried using TCS's help. Since we provided our own app_id for this to happen, which will be resolved once we finally do this by default for OAuth Flow in Cloud.

Therefore the decision to hide it until the OAuth Flow is fixed has been made. @jnr0790 Please confirm.

Was the app_id only used for oauth? If not, does this have any effect on OSS users that would have passed app_id in their config?

It's required only for OAuth as a part of the advancedAuth in spec, the OSS users are not affected. Tested.

pedroslopez

Did not functionally test this, but directionally looks good to me. Thanks for clarifying the breaking change questions - since this won't affect existing users I don't think we need any outreach here.

Left some questions on the acceptance test config to understand why our tests marked this as breaking. Non-blocking though.

We should make sure everything works with the changes outlined on the server-side PR (not hardcoding the app_id) before merging this in.

airbyte-integrations/connectors/source-amazon-seller-partner/metadata.yaml

airbyte-integrations/connectors/source-amazon-seller-partner/acceptance-test-config.yml

bazarnov · 2023-06-12T21:38:29Z

Did not functionally test this, but directionally looks good to me. Thanks for clarifying the breaking change questions - since this won't affect existing users I don't think we need any outreach here.

Left some questions on the acceptance test config to understand why our tests marked this as breaking. Non-blocking though.

We should make sure everything works with the changes outlined on the server-side PR (not hardcoding the app_id) before merging this in.

i'll make the server side change tomorrow and provide the results in the related PR.

bazarnov · 2023-06-13T12:04:04Z

Server-side PR: https://github.com/airbytehq/airbyte-platform-internal/pull/7085 has been updated and tested. Works as expected using app_id from instance-wide params.

bazarnov · 2023-06-14T14:39:08Z

/test connector=connectors/source-amazon-seller-partner

🕑 connectors/source-amazon-seller-partner https://github.com/airbytehq/airbyte/actions/runs/5268571820
✅ connectors/source-amazon-seller-partner https://github.com/airbytehq/airbyte/actions/runs/5268571820
Python tests coverage:

Name                                        Stmts   Miss  Cover
---------------------------------------------------------------
source_amazon_seller_partner/__init__.py        2      0   100%
source_amazon_seller_partner/constants.py      38      1    97%
source_amazon_seller_partner/source.py         43     11    74%
source_amazon_seller_partner/streams.py       583    242    58%
source_amazon_seller_partner/auth.py           61     36    41%
---------------------------------------------------------------
TOTAL                                         727    290    60%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:109: Backward compatibility tests are disabled for version 1.2.0.
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.2.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.
================== 38 passed, 3 skipped in 2470.54s (0:41:10) ==================

octavia-squidington-iii · 2023-06-15T01:15:49Z

source-amazon-seller-partner test report (commit `0d16ab009b`) - ✅

⏲️ Total pipeline duration: 2558 seconds

Step	Result
Validate airbyte-integrations/connectors/source-amazon-seller-partner/metadata.yaml	✅
Connector version semver check.	✅
Connector version increment check.	✅
QA checks	✅
Connector package install	✅
Build source-amazon-seller-partner 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-seller-partner test

animer3009 · 2023-06-23T15:06:05Z

@bazarnov
Where can I add app_id?
I have variable app_ids.
Early I was creating connectors using an API including app_id.
Now it pulls 0 rows after upgrading.
Urgent to solve that as I have that on production.
Please share some instructions...
I can't find where advanced_auth is...

bazarnov · 2023-06-23T16:58:36Z

@animer3009 Are you a Cloud user? why do you need the app id, it's not used elsewhere in the connector, therefore it was removed from the spec, as of it was needed just to obtain the access token, using OAuth 2.0 authentication option. We are now making the connection for OAuth 2.0 using internal app_id which is hidden from the users completely.

please share the screenshots from the ui you're using?
as for the emergency and you're using OSS, you're able to switch back to the previous version.
please share the logs of your latest sync with 0 records.

thank you

animer3009 · 2023-06-23T17:13:13Z

@bazarnov
Nope, I am not the cloud user.

Now I custom builded connector. Returned back app_id.
Testing it right now....

I am using Airbyte 0.50.4

log: b464d45a_4683_4c4c_8254_f9c72f5a24ed_logs_17482_txt.txt

I am not sure if downgrade only connector will help. First I will try custom builded connector.

animer3009 · 2023-06-23T17:32:31Z

@bazarnov
Same with custom build.
Now I will try downgrade connector only. If it will not help, then will downgrade airbyte too..
I will keep you updated.

What I noticed is, looks like Airbyte does not wait for Amazon report generation...
It finish in seconds...

bazarnov · 2023-06-23T17:58:58Z

@animer3009 It's odd, because the last updates i've made to this connector didn't touch the fetch logic at all.

Could you verify whether or not any rate limits hit recently? as well as start date, please move it to a week ago, for instance.

I believe there is nothing to do with the app_id here.

animer3009 · 2023-06-23T18:12:20Z

@bazarnov
Possibly.
Yeah, downgrading Amazon Seller Partner to 1.2.0 does not help.
Then I downgraded Airbyte itself to the point where from I upgraded (1 release up) 0.44.12.
Before the upgrade it was 0.44.4...

Looks like it still started pulling... :)
And yeah in case of downgrading only the connector, it still finished in seconds. Filling was like it was not waiting for the ASP rapport generation.

My VM is production. I can't play with it, as it is still working, I will keep it as is.
But I will try to make a test VM and play there.

bazarnov · 2023-06-23T18:38:07Z

@animer3009 I strongly recommend you to create an issue for this, and mention about the upgrading Airbyte, which caused problems, this should be investigated further. Thanks.

animer3009 · 2023-07-06T20:15:09Z

@bazarnov
UPDATE:
Unfortunately we were not able to repeat the issue on the test server.
We were not able to test all previous releases but reason possibly can be one of them.
One thing we did differently is that we keep our .ini and
docker-compose.yaml file instead of pulling a fresh one.

P.s.
What I found during tests is that local build (dev) still include app_id optional input.
I am not sure if that is OK (just FYI).

Now we decided to still upgrade our production server.
We will observe stuff and keep you updated.

animer3009 · 2023-07-06T20:34:00Z

@bazarnov
same issue on production. looks like something wrong with API, as we create connector connections using Airbyte API on production.
Still troubleshooting.

animer3009 · 2023-07-06T22:04:59Z

@bazarnov
I found the reason.
Looks like changed core logic and streams are not auto selected any more.
So I hardcoded selected = true on my end.
Now it is still working :)
Possibly guys who are using API calls will face the same issue. But don't think that there will be a lot of them lol :)

bazarnov · 2023-07-07T07:41:54Z

nicely done!

added to the OAuth input spec by default

fd71580

bazarnov self-assigned this Jun 7, 2023

octavia-squidington-iii added area/connectors Connector related issues connectors/source/amazon-seller-partner labels Jun 7, 2023

bazarnov commented Jun 7, 2023

View reviewed changes

bazarnov changed the title ~~🐛 Source Amazon Seller Partner: add app_id to the OAuth Flow by default~~ 🐛 Source Amazon Seller Partner: add app_id to the OAuth Flow (source spec side) Jun 7, 2023

bazarnov added the checklist-action-run label Jun 8, 2023

bazarnov added 4 commits June 8, 2023 23:43

updated

2f51751

Merge remote-tracking branch 'origin/master' into baz/source-amazon-s…

309651b

…eller-partner-oauth-fix

added missing spec

3863b11

updated

d38d6fb

updated changelog

a088445

octavia-squidington-iii added the area/documentation Improvements or additions to documentation label Jun 9, 2023

bazarnov requested review from davydov-d, lazebnyi, roman-yermilov-gl and a team June 9, 2023 09:39

bazarnov added 2 commits June 9, 2023 14:02

Merge remote-tracking branch 'origin/master' into baz/source-amazon-s…

d15dd29

…eller-partner-oauth-fix

added missing spec

2143e01

updated acceptance-test-config

9defce5

pedroslopez self-requested a review June 9, 2023 15:17

bazarnov added 3 commits June 9, 2023 23:41

Merge branch 'master' into baz/source-amazon-seller-partner-oauth-fix

45992be

updated Orders schema

dccb8d3

updated schema again

cb5dcbf

lazebnyi requested changes Jun 12, 2023

View reviewed changes

bazarnov requested a review from lazebnyi June 12, 2023 12:29

Merge remote-tracking branch 'origin/master' into baz/source-amazon-s…

0f8d9ba

…eller-partner-oauth-fix

lazebnyi approved these changes Jun 12, 2023

View reviewed changes

pedroslopez reviewed Jun 12, 2023

View reviewed changes

bazarnov requested a review from pedroslopez June 12, 2023 20:49

pedroslopez approved these changes Jun 12, 2023

View reviewed changes

Merge branch 'master' into baz/source-amazon-seller-partner-oauth-fix

0d16ab0

bazarnov merged commit 04a8267 into master Jun 15, 2023

bazarnov deleted the baz/source-amazon-seller-partner-oauth-fix branch June 15, 2023 07:29

bazarnov mentioned this pull request Jun 15, 2023

Source Amazon Seller Partner: problems with the 'Authenticate Seller Partner account' stage (Alpha version) #25773

Closed

1 task

fitzyjoe mentioned this pull request Feb 5, 2025

[source-amazon-seller-partner] OAuth fails silently, missing app_id #53181

Open

1 task

🐛 Source Amazon Seller Partner: add app_id to the OAuth Flow (source spec side) #27110

🐛 Source Amazon Seller Partner: add app_id to the OAuth Flow (source spec side) #27110

Conversation

bazarnov commented Jun 7, 2023 • edited Loading

What

How

🚨 User Impact 🚨

github-actions bot commented Jun 7, 2023 • edited by bazarnov Loading

Before Merging a Connector Pull Request

bazarnov Jun 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bazarnov commented Jun 8, 2023 • edited Loading

bazarnov commented Jun 9, 2023 • edited Loading

bazarnov commented Jun 9, 2023

bazarnov commented Jun 10, 2023 • edited by github-actions bot Loading

Build Passed

Choose a reason for hiding this comment

bazarnov commented Jun 12, 2023 • edited by github-actions bot Loading

Build Passed

lazebnyi left a comment

Choose a reason for hiding this comment

pedroslopez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bazarnov commented Jun 12, 2023

pedroslopez commented Jun 12, 2023

bazarnov commented Jun 12, 2023 • edited Loading

pedroslopez left a comment

Choose a reason for hiding this comment

bazarnov commented Jun 12, 2023

bazarnov commented Jun 13, 2023

bazarnov commented Jun 14, 2023 • edited by github-actions bot Loading

Build Passed

octavia-squidington-iii commented Jun 15, 2023

source-amazon-seller-partner test report (commit 0d16ab009b) - ✅

animer3009 commented Jun 23, 2023

bazarnov commented Jun 23, 2023 • edited Loading

animer3009 commented Jun 23, 2023 • edited Loading

animer3009 commented Jun 23, 2023 • edited Loading

bazarnov commented Jun 23, 2023

animer3009 commented Jun 23, 2023

bazarnov commented Jun 23, 2023

animer3009 commented Jul 6, 2023 • edited Loading

animer3009 commented Jul 6, 2023

animer3009 commented Jul 6, 2023

bazarnov commented Jul 7, 2023

🐛 Source Amazon Seller Partner: add `app_id` to the OAuth Flow (source `spec` side) #27110

🐛 Source Amazon Seller Partner: add `app_id` to the OAuth Flow (source `spec` side) #27110

bazarnov commented Jun 7, 2023 •

edited

Loading

github-actions bot commented Jun 7, 2023 •

edited by bazarnov

Loading

bazarnov Jun 7, 2023 •

edited

Loading

bazarnov commented Jun 8, 2023 •

edited

Loading

bazarnov commented Jun 9, 2023 •

edited

Loading

bazarnov commented Jun 10, 2023 •

edited by github-actions bot

Loading

bazarnov commented Jun 12, 2023 •

edited by github-actions bot

Loading

bazarnov commented Jun 12, 2023 •

edited

Loading

bazarnov commented Jun 14, 2023 •

edited by github-actions bot

Loading

source-amazon-seller-partner test report (commit `0d16ab009b`) - ✅

bazarnov commented Jun 23, 2023 •

edited

Loading

animer3009 commented Jun 23, 2023 •

edited

Loading

animer3009 commented Jun 23, 2023 •

edited

Loading

animer3009 commented Jul 6, 2023 •

edited

Loading