Tdl 681 add retry mechanism #30
Conversation
tap_klaviyo/utils.py
Outdated
|
|
||
| def raise_for_error(response): | ||
| try: | ||
| json_resp = response.json() |
There was a problem hiding this comment.
@harshpatel4crest Can you please get the response of response.raise_for_status() here and except the HTTTError in the exception.
Please refer to the tap-Xero code (https://github.com/singer-io/tap-xero/pull/85/files#diff-43d32c99c6992e01f4dbdb49df5e372db029dae0de306b9191283ef21b852438R216) for the same
tests/test_exception_handling.py
Outdated
| import singer | ||
|
|
||
| class Mockresponse: | ||
| def __init__(self, resp, status_code, content=None, headers=None, raise_error=False): |
There was a problem hiding this comment.
@harshpatel4crest You can make resp an optional argument.
class Mockresponse:
def __init__(self, status_code, resp={}, content=None, headers=None, raise_error=False):
self.json_data = resp
...
def klaviyo_400_error(*args, **kwargs):
return Mockresponse(json_str, 400, raise_error=True)
Please follow the above code for the remaining functions
| class TestBackoff(unittest.TestCase): | ||
|
|
||
| @mock.patch('requests.Session.request', side_effect=klaviyo_400_error) | ||
| def test_400_error(self, klaviyo_400_error): |
There was a problem hiding this comment.
@harshpatel4crest Can you also add a positive scenario as well
tap_klaviyo/utils.py
Outdated
| }, | ||
| 403: { | ||
| "raise_exception": KlaviyoForbiddenError, | ||
| "message": "Request is missing or has an invalid API key." |
There was a problem hiding this comment.
Can we adjust the message here to say "Invalid authorization credentials or permissions."
There was a problem hiding this comment.
I have added the desired string, however I have taken the string from Klaviyo Documentation https://www.klaviyo.com/docs/api/lists.
So, let me know if want to revert the change.
tap_klaviyo/utils.py
Outdated
| }, | ||
| 500: { | ||
| "raise_exception": KlaviyoInternalServiceError, | ||
| "message": "Something is wrong on Klaviyo's end." |
There was a problem hiding this comment.
Can we also adjust this to say "Internal Service Error from Klaviyo"
There was a problem hiding this comment.
Added the string in message
|
Can you resolve the conflicts? |
@KAllan357 |
Description of change
Manual QA steps
Risks
Rollback steps