-
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-amazon-seller-partner: allow to use IAM user arn or IAM role arn #12523
source-amazon-seller-partner: allow to use IAM user arn or IAM role arn #12523
Conversation
Thank you @pecalleja for this contribution. I'm running the tests and will get back for a first review once they pass. |
/test connector=connectors/source-amazon-seller-partner
|
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.
Minor suggestions
...-integrations/connectors/source-amazon-seller-partner/source_amazon_seller_partner/source.py
Outdated
Show resolved
Hide resolved
...-integrations/connectors/source-amazon-seller-partner/source_amazon_seller_partner/source.py
Show resolved
Hide resolved
...-integrations/connectors/source-amazon-seller-partner/source_amazon_seller_partner/source.py
Outdated
Show resolved
Hide resolved
...-integrations/connectors/source-amazon-seller-partner/source_amazon_seller_partner/source.py
Outdated
Show resolved
Hide resolved
...-integrations/connectors/source-amazon-seller-partner/source_amazon_seller_partner/source.py
Outdated
Show resolved
Hide resolved
...-integrations/connectors/source-amazon-seller-partner/source_amazon_seller_partner/source.py
Outdated
Show resolved
Hide resolved
...-integrations/connectors/source-amazon-seller-partner/source_amazon_seller_partner/source.py
Outdated
Show resolved
Hide resolved
...-integrations/connectors/source-amazon-seller-partner/source_amazon_seller_partner/source.py
Outdated
Show resolved
Hide resolved
...-integrations/connectors/source-amazon-seller-partner/source_amazon_seller_partner/source.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/source-amazon-seller-partner/unit_tests/test_source.py
Outdated
Show resolved
Hide resolved
do you want I include more unit tests in this PR to fully cover the connector source code? |
…ource_amazon_seller_partner/source.py use a better exception text message Co-authored-by: Augustin <augustin@airbyte.io>
…ource_amazon_seller_partner/source.py use better variable name Co-authored-by: Augustin <augustin@airbyte.io>
I'd prefer you only add tests for the code you add. Feel free to open another PR if you want to increase the coverage of other functions. |
/test connector=connectors/source-amazon-seller-partner
|
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 @pecalleja, we are close to having a mergeable PR 😄
I need to ask you some extra work to make your change available in the published connector:
- Bump the connector version in
Dockerfile
airbyte-config/init/src/main/resources/seed/source_definitions.yaml
airbyte-config/init/src/main/resources/seed/source_specs.yaml
Update the changelog in docs/integrations/sources/amazon-seller-partner.md
I can't do these changes myself as I've no permission to push on your branch.
Could you also please merge master into your branch? This is probably the reason the latest test fail. |
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.
requested changes implemented 👍. Thank you.
/test connector=connectors/source-amazon-seller-partner
|
airbyte-integrations/connectors/source-amazon-seller-partner/unit_tests/test_source.py
Show resolved
Hide resolved
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.
LGTM
/test connector=connectors/source-amazon-seller-partner |
/test connector=connectors/source-amazon-seller-partner
|
/publish connector=connectors/source-amazon-seller-partner auto-bump-version=false
|
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 @pecalleja for your changes!
What
The amazon sp-api integration allows to use IAM user entity or IAM role entity. amazon documentation
if we use a user based arn we need to use another function from the boto3 client to get the required credentials role
This will allow to integrate with airbyte apps configured with user arn entity in the amazon seller central app.
How
just use the required function for differents IAM entities
🚨 User Impact 🚨
None
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 exampleTests
Unit
Put your unit tests output here.
Integration
Put your integration tests output here.
Acceptance
Put your acceptance tests output here.