-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
fix(source-google-ads): fix attribute error on check connection #42971
fix(source-google-ads): fix attribute error on check connection #42971
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Skipped Deployment
|
/approve-regression-tests
|
@@ -225,6 +225,8 @@ def check_connection(self, logger: logging.Logger, config: Mapping[str, Any]) -> | |||
customer_id=customer.id, | |||
login_customer_id=customer.login_customer_id, | |||
) | |||
except AirbyteTracedException as e: | |||
raise e |
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'm a little bit worried about losing the underlying context here, probably we should raise something like:
raise e from e
But if it is OK, and we don't lose the context of the AirbyteTracedException
raised prior, we should be fine with this.
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.
Thanks, updated
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
|
What
resolved: https://github.com/airbytehq/oncall/issues/6141
How
We should handle AirbyteTracedException on check separate from Google Ads Exception to avoid attribute error when AirbyteTracedException is passed to traced_exception method.
Review guide
User Impact
Can this PR be safely reverted and rolled back?