Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: Add option to configure HTTP status codes to consider as failure #82

Merged
merged 3 commits into from
Jun 1, 2021
Merged

feat: Add option to configure HTTP status codes to consider as failure #82

merged 3 commits into from
Jun 1, 2021

Conversation

pvgnd
Copy link
Contributor

@pvgnd pvgnd commented Mar 25, 2021 •
edited
Loading

closes #73

TODO:

  • Add HttpClient implementation (+ tests + doc)
  • Add Guzzle implementation (+ tests + doc)

@pvgnd pvgnd marked this pull request as draft March 25, 2021 08:07
@coveralls
Copy link

coveralls commented Mar 25, 2021 •
edited
Loading

Coverage Status

Coverage increased (+0.5%) to 92.197% when pulling 218b8e8 on pvgnd:failure-conf into aa4fa2b on ackintosh:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 92.033% when pulling 627e516 on pvgnd:failure-conf into aa4fa2b on ackintosh:master.

@pvgnd pvgnd force-pushed the failure-conf branch 2 times, most recently from 0927fb6 to 9fc9076 Compare March 28, 2021 08:51
@pvgnd pvgnd changed the title feat: Add option to configure HTTP status codes to consider as failure feat(GaneshaHttpClient): Add option to configure HTTP status codes to consider as failure Mar 28, 2021
@pvgnd pvgnd changed the title feat(GaneshaHttpClient): Add option to configure HTTP status codes to consider as failure feat: Add option to configure HTTP status codes to consider as failure Mar 28, 2021
@pvgnd
Copy link
Contributor Author

pvgnd commented Mar 28, 2021 •
edited
Loading

@ackintosh could you have a look on the HttpClient implementation before I start to work on the Guzzle implementation?

@ackintosh
Copy link
Owner

@pvgnd Thank you! I will have a look this weekend or next weekend. I'm sorry to have kept you waiting. 🙇

ackintosh
ackintosh reviewed May 17, 2021
View reviewed changes
Copy link
Owner

@ackintosh ackintosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left some small comments!

README.md Outdated Show resolved Hide resolved
src/Ganesha/GaneshaHttpClient.php Outdated Show resolved Hide resolved
src/Ganesha/HttpClient/RestFailureDetector.php Outdated Show resolved Hide resolved
@pvgnd pvgnd marked this pull request as ready for review May 26, 2021 19:26
@ackintosh
Copy link
Owner

@pvgnd Thank you for the fix! Looks good to me. ✨

@pvgnd
Copy link
Contributor Author

pvgnd commented Jun 1, 2021

@pvgnd Thank you for the fix! Looks good to me. ✨

Thanks for the review. I suggest to add same option to Guzzle Middleware but in another PR. What do you think?

@ackintosh
Copy link
Owner

I agree with you, will merge this PR.

@ackintosh ackintosh merged commit 81fd038 into ackintosh:master Jun 1, 2021
@ackintosh
Copy link
Owner

@pvgnd The feature has been released as v1.3.0. 🎉 Thank you for your contribution! :octocat:

https://github.com/ackintosh/ganesha/releases/tag/1.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ackintosh ackintosh ackintosh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[GuzzleMiddleware + GaneshaHttpClient] Option to trigger failure on 5xx response
3 participants
@pvgnd @coveralls @ackintosh