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

Customize health syncing status via query parameter #4013

Merged

Conversation

jmcruz1983
Copy link
Contributor

@jmcruz1983 jmcruz1983 commented May 19, 2021

Customize health check status when node is syncing

  • use query parameter to override default syncing status
  • return default status if wrong parameter passed

Documentation

Health check syncing status can be overridden with custom value passed via query parameter syncing_status.
Therefore, it would be compatible with kubernetes standard health check where 200-399 statuses are considered healthy.

Changelog

  • I thought about adding a changelog entry, and added one if I deemed necessary.

@CLAassistant
Copy link

CLAassistant commented May 19, 2021

CLA assistant check
All committers have signed the CLA.

@jmcruz1983 jmcruz1983 changed the title Customize health syncing status via parameter Customize health syncing status via query parameter May 19, 2021
Copy link
Contributor

@rolfyone rolfyone left a comment

Choose a reason for hiding this comment

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

LGTM.
If this is a helpful change more generally, could potentially look at making a spec change on eth2.0-apis so that it's part of the standard api...

@rolfyone
Copy link
Contributor

@jmcruz1983 could you take a look at the CLA for me? thanks...

@jmcruz1983 jmcruz1983 force-pushed the customize-healthcheck-syncing-status branch from eb986da to 2f7b91c Compare May 20, 2021 11:46
@jmcruz1983
Copy link
Contributor Author

FYI @rolfyone
I did sign the CLA,
but don't know why still shows unsigned.

@ajsutton
Copy link
Contributor

I did sign the CLA,
but don't know why still shows unsigned.

Yeah it has approved your GitHub user but doesn't seem to be able to map the commits to that user. From the comment it left above:

Juan Dominguez seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

@ajsutton
Copy link
Contributor

May need to edit the commit so it has an email address with it (can just be the one GitHub generates rather than your real one) and do a force push to replace the commit.

@jmcruz1983
Copy link
Contributor Author

jmcruz1983 commented May 20, 2021

Thanks @ajsutton
I managed to get it right,
seems that commit was using my GitLab user instead of GitHub.
Now CLA is signed :)

- use request parameter to override default syncing status
- return default status if wrong parameter passed
- add tests to cover different scenarios
@jmcruz1983 jmcruz1983 force-pushed the customize-healthcheck-syncing-status branch from 2f7b91c to 813da8f Compare May 20, 2021 14:09
@rolfyone rolfyone merged commit af6eea5 into Consensys:master May 20, 2021
@jmcruz1983 jmcruz1983 deleted the customize-healthcheck-syncing-status branch May 20, 2021 21:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants