Skip to content
This repository has been archived by the owner on May 16, 2023. It is now read-only.

issue-60: Add a configurable health status to the readinessProbe command #62

Merged

Conversation

rwittrick
Copy link
Contributor

No description provided.

@elasticmachine
Copy link
Collaborator

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

Copy link
Contributor

@Crazybus Crazybus left a comment

Choose a reason for hiding this comment

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

This looks great! I had one suggestion about making this a bit more generic so that we don't end up with lots and lots of variables to customise other potential params that users might want to tweak.

@@ -118,6 +118,9 @@ readinessProbe:
successThreshold: 3
timeoutSeconds: 5

# https://www.elastic.co/guide/en/elasticsearch/reference/current/cluster-health.html#request-params wait_for_status
minClusterHealthStatus: green
Copy link
Contributor

Choose a reason for hiding this comment

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

There are so many custom paramters here that people might want to change other than wait_for_status. How about making this more generic and having it be something like:

clusterHealthCheckParams: wait_for_status=green&timeout=1s

This is going to make this a lot more maintainable if users want to modify the timeout...or add other settings, or if a feature release of Elasticsearch changes the parameters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated to that format.

elasticsearch/templates/statefulset.yaml Outdated Show resolved Hide resolved
Copy link
Contributor

@Crazybus Crazybus left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for adding this in!

@Crazybus
Copy link
Contributor

jenkins test this please

@Crazybus Crazybus merged commit 35c87aa into elastic:master Feb 15, 2019
@rwittrick rwittrick deleted the issue-60-configurable-health-status branch February 15, 2019 15:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants