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.

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jackhammer2k
Copy link

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".

@csabakos
Copy link

+1 for this issue. It would be awesome to get this merged :)

@jackhammer2k
Copy link
Author

Any comment from the maintainers? Does anyone know how to fix the failed build?

@TYsewyn
Copy link
Contributor

TYsewyn commented Dec 10, 2019

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,
Copy link
Contributor

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,
Copy link
Contributor

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(
Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Member

@spencergibb spencergibb left a 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

@jackhammer2k
Copy link
Author

Looks like there is a sporadically failing test (I did not touch the failing module). Could you please restart the tests?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants