-
Notifications
You must be signed in to change notification settings - Fork 860
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
refactor: http status code and error in api(daytonaio#934) #1056
refactor: http status code and error in api(daytonaio#934) #1056
Conversation
3a433e9
to
335239c
Compare
@Tpuljak Don't know why tests are failing. |
335239c
to
6ddecdc
Compare
Because it seems you didn't set up the mock correctly. Before pushing, please run |
6ddecdc
to
ef32806
Compare
@Tpuljak I tried everything but still is not working. I am returning HTTP Status Code as 2nd return value of the functions. |
@the-johnwick a glanced at the code and I suggest that we go a different route for this. Returning an I suggest the following:
I'll put this PR in draft now. You can mark it ready for review once you've implemented the suggestions. Thanks! |
@Tpuljak I will update it ASAP. Thanks for the help. |
8f26e5c
to
f818101
Compare
@Tpuljak Done Please check. |
@the-johnwick putting this back in draft. Please make sure to test everything before requesting a review again.
|
@Tpuljak do you mean to format git provider client errors only right |
Yes. |
5defd6a
to
b8835cf
Compare
Signed-off-by: the-johnwick <[email protected]>
b8835cf
to
3925658
Compare
@Tpuljak Done updated the all the changes. |
Signed-off-by: the-johnwick <[email protected]>
3f859b6
to
ac69a6d
Compare
Signed-off-by: the-johnwick <[email protected]>
@Tpuljak Updated the changes. |
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.
Signed-off-by: the-johnwick <[email protected]>
@Tpuljak Updated the code. Added regex instead of marshaling/unmarshalling. |
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.
@the-johnwick did you test all the errors for all providers before requesting the review?
@Tpuljak Retested everything and fixed the problem |
3dbf32f
to
d5fc60e
Compare
Signed-off-by: the-johnwick <[email protected]>
d5fc60e
to
206281e
Compare
Signed-off-by: the-johnwick <[email protected]>
d60dc2f
to
88a3c6f
Compare
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.
Signed-off-by: the-johnwick <[email protected]>
691593c
to
d144b34
Compare
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.
Looks good. Nice work!
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!
@Tpuljak Can we merge it now? |
…#1056) Signed-off-by: the-johnwick <[email protected]> Signed-off-by: bryans-go <[email protected]>
Refactor better HTTP status code and error
Description
Returns client-side HTTP response code & adds error messages according to the error.
issue #934
/claim #934