Allow setting server-wide status via StaticChecker
#75
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Josh Humphries <[email protected]>
jhump
force-pushed
the
jh/allow-setting-server-wide-status
branch
from
79b3f3f to
0e7d326
Compare
… no service name is specified in check request) Signed-off-by: Josh Humphries <[email protected]>
jhump
force-pushed
the
jh/allow-setting-server-wide-status
branch
from
0e7d326 to
b7dd413
Compare
emcfarlane
approved these changes
Jan 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, the
StaticCheckerwould allow calling code to change the status for a service named"", but it was effectively discarded (it was actually stored in a map, but never queried). The handling of requests for the empty-named service (which returns a "server-wide" status instead of a service-specific status) never bothered to look at the checker's map.This change updates the tests to demonstrate the issue and then fixes it by having the handler first look at the map to see if a server-wide status is set. It then defaults to
Servingonly if nothing is found in the map.This is not particularly ergonomic (since it feels like the server-wide status should somehow be derived from and/or impact the per-service statuses), but the intent is not to make
StaticCheckermore sophisticated but just to remove what appears to be a bug. It could be argued that it's not a bug and that a documentation update might suffice, but the current behavior is counter-intuitive, so it seemed best to change it.