-
Notifications
You must be signed in to change notification settings - Fork 109
improved Mandatory ID Filter to return info upon rejection #751
Conversation
also refactored MandatoryClientId & Shutdown Filters using the Template Method design pattern as they were very similar
Codecov Report
@@ Coverage Diff @@
## master #751 +/- ##
============================================
+ Coverage 55.02% 55.13% +0.11%
- Complexity 3153 3162 +9
============================================
Files 746 748 +2
Lines 20389 20395 +6
Branches 1339 1339
============================================
+ Hits 11219 11245 +26
+ Misses 8682 8662 -20
Partials 488 488
Continue to review full report at Codecov.
|
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.
worked like a charm locally:
{"message":"Anonymous requests are not permitted","reason":"Service Unavailable","status":503,"type":"authentication-error"}%
nice :)
heroic-core/src/main/java/com/spotify/heroic/http/HttpServer.java
Outdated
Show resolved
Hide resolved
One question I had, why the switch from 400 to 503? |
heroic-core/src/main/java/com/spotify/heroic/servlet/MandatoryClientIdFilter.java
Outdated
Show resolved
Hide resolved
heroic-core/src/main/java/com/spotify/heroic/servlet/MandatoryClientIdFilter.java
Outdated
Show resolved
Hide resolved
heroic-core/src/main/java/com/spotify/heroic/ws/MandatoryClientIdErrorMessage.java
Outdated
Show resolved
Hide resolved
|
Hey @lmuhlha , @samfadrigalan could one of you please approve - I addressed all the issues raised. Cheers. |
@sming One quick thought -- It might be a good idea to update the public docs for /query/metrics to stipulate the header requirement. It's currently part of the sample cURL but not called out as a requirement. |
very similar.
Note for reviewers
Checkout out the Template Method design pattern before reviewing this PR 👍🏻