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

Proposal : Overhaul readiness checks #212

Merged

Conversation

n1ru4l
Copy link
Collaborator

@n1ru4l n1ru4l commented Feb 25, 2021

This is a breaking change the overhauls the existing readiness check system. All readiness check related stuff is now exported via "dockest/readiness-check".

By default a readiness check does no longer have a retry. Retry must be explicitly declared by wrapping a readiness check with the withRetry function exported from "dockest/readiness-check".

import { withRetry } from "dockest/readiness-check";
const readinessCheck = async () => {
  throw new Error("Will always fail.")
}

const readinessCheckWithRetry = withRetry(readinessCheck, 10);

Having a global retry does make sense for brute-force attempts (e.g. try run command 10 times before failing), but not for readiness checks that observe the docker event stream or potentially in the future logs.

The old readiness checks for redis, postgres, and web are now also exported from that namespace. Those already have the retry "trait" applied. So it is a bit easier to upgrade, if anyone is already relying on that readinesschecks.

import {
  createWebReadinessCheck,
  createRedisReadinessCheck,
  createPostgresReadinessCheck,
} from "dockest/readiness-check"

The zeroExitCodeReadinessCheck assumes that a container is ready when it exits with the exit code "0". This is useful if you have a container that is responsible for running database migrations.

The containerIsHealthyReadinessCheck assumes that a container is ready once the docker health check pubishes a "heathy" value via the docker event stream. This is IMHO a better and universal way of verifying that a container is ready (in comparison to the old redis, postgres, and web retry-readiness checks), as many containers already come with a health-check definition in their Dockerfile. Most setups should therefore be best off by using that readiness check.

The withNoStop utility can be used to automatically fail any user-land readiness check in case the container exists preliminary. It is also used by the containerIsHealthyReadinessCheck, createWebReadinessCheck, createRedisReadinessCheck and createPostgresReadinessCheck for faster feedback (instead of waiting for a retry to time out).

@n1ru4l n1ru4l changed the title Proposal retry fail in user land Proposal : Overhaul readiness checks Feb 25, 2021
@n1ru4l n1ru4l marked this pull request as ready for review February 25, 2021 13:50
@erikengervall
Copy link
Owner

Finally had a chance to review this - looks good @n1ru4l 👍

@erikengervall
Copy link
Owner

Let's get this and #213 into a new major

@n1ru4l
Copy link
Collaborator Author

n1ru4l commented Mar 5, 2021

we are also already using these changes successfully in our ci pipelines on our private fork :)

@erikengervall
Copy link
Owner

we are also already using these changes successfully in our ci pipelines on our private fork :)

haha, happy to hear it's been pre-beta tested 😄

@erikengervall
Copy link
Owner

erikengervall commented Mar 5, 2021

@n1ru4l do you have permissions to merge PRs once approved?

@n1ru4l
Copy link
Collaborator Author

n1ru4l commented Mar 5, 2021

@erikengervall nope 😅

@erikengervall
Copy link
Owner

@erikengervall nope 😅

alright, gimmie a minute to dig around in settings haha

@erikengervall
Copy link
Owner

@n1ru4l so i've set a requirement for 1 review on protected branches, which is only only master currently

i saw u accepted my invite to become a collaborator, i hope this enables the shiny merge button 🤞

@n1ru4l n1ru4l merged commit f1a4b60 into erikengervall:master Mar 5, 2021
@n1ru4l n1ru4l deleted the proposal-retry-fail-in-user-land branch March 5, 2021 21:28
@n1ru4l
Copy link
Collaborator Author

n1ru4l commented Mar 5, 2021

awesome :)

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.

2 participants