-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
ref(source-stripe): remove stripe py-package #45348
Conversation
[skip ci] Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Regression tests failed only for control version. |
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 think we should ensure that we don't prevent alerting when needed. The rest looks good to me
stream_is_available, reason = availability_strategy.check_availability(account_stream, logger) | ||
if not stream_is_available: | ||
return False, reason | ||
except Exception as error: |
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.
If we catch all the exceptions here, we might hide some system errors that should have trigger paging. We should probably do something similar as this PR where we only catch AirbyteTracedException of type config error
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 reused logic from Declarative Stream check and HttpAvailabilityStrategy. Seems like we need to redo declarative check as well.
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.
Yeah... rip! For the HttpAvailabilityStrategy, I think this is known and we discussed that we would move away from it as we would delete the availability strategy. I don't know where we are at with this. For the DeclarativeStream, it seems like a problem too.
@@ -107,12 +108,26 @@ def validate_and_fill_with_defaults(config: MutableMapping[str, Any]) -> Mutable | |||
return config | |||
|
|||
def check_connection(self, logger: logging.Logger, config: MutableMapping[str, Any]) -> Tuple[bool, Any]: | |||
self.validate_and_fill_with_defaults(config) | |||
stripe.api_key = config["client_secret"] | |||
config = self.validate_and_fill_with_defaults(config) |
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.
nit: the logic to prepare the config seems very similar than the one in streams
. Should we extract that?
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.
agreed, done
[skip ci] Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
Signed-off-by: Artem Inzhyyants <artem.inzhyyants@gmail.com>
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!
/approve-regression-tests
|
This PR does not allow for tokens that do not have read access to all platform connected accounts. I am getting the following error. The endpoint https://api.stripe.com/v1/accounts?limit=100 returned 400: Bad Request. You cannot access the connected accounts of your platform's connected accounts.. Please visit https://docs.airbyte.com/integrations/sources/stripe to learn more. You cannot access the connected accounts of your platform's connected accounts. |
What
refactor to remove python package
stripe
Reason:
check
and can be replaced with existingHttpStream
implementation.How
remove python stripe package
Review guide
airbyte-integrations/connectors/source-stripe/source_stripe/source.py
User Impact
Can this PR be safely reverted and rolled back?