-
Notifications
You must be signed in to change notification settings - Fork 541
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
add option to configure Consul check status considered as healthy #599
base: main
Are you sure you want to change the base?
add option to configure Consul check status considered as healthy #599
Conversation
+1 for this issue. It would be awesome to get this merged :) |
Any comment from the maintainers? Does anyone know how to fix the failed build? |
It looks like your fork is some commits behind our master. Can you please merge our changes into your feature branch and try again? |
*/ | ||
protected List<ConsulServer> transformResponse(List<HealthService> healthServices) { | ||
protected List<ConsulServer> transformResponse(List<HealthService> healthServices, |
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.
To avoid changing the method signature I would use this.properties
in the method rather than pass in the Set
. You can add an overloaded method if you want to give the option to past in the Set
private final Map<String, String> metadata; | ||
|
||
public ConsulServer(final HealthService healthService) { | ||
public ConsulServer(final HealthService healthService, |
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.
We need to maintain backwards compatibility so we need to leave this constructor as is (perhaps mark it as deprecated) and add a new one.
/** | ||
* List of status considered as healthy. Contains "passing" by default. | ||
*/ | ||
private Set<Check.CheckStatus> statusConsideredAsHealthy = new HashSet( |
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.
We should document the new property
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.
You can add the explanation of this feature to docs/src/main/asciidoc/spring-cloud-consul.adoc
.
docs/src/main/asciidoc/_configprops.adoc
will be automatically generated for you so you don't need to make any changes here.
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.
I agree with my colleagues comments
* restore backwards-compatibility of changed classes
Looks like there is a sporadically failing test (I did not touch the failing module). Could you please restart the tests? |
Instead of just considering check status "passing" as healthy we make the status configurable. That allows us to also consider e.g. "warning" as healthy.
Default is the current behavior that only allows "passing".