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

Changed default value of the startInterval to 5s #47799

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

j2walker
Copy link

@j2walker j2walker commented May 5, 2024

- What I did
Changed the startInterval default value to be 5s

- How I did it
Added a new variable named defaultStartInterval and set it to 5s. Before, the default value was set to proveInterval by accident.

- How to verify it
Run the container's monitoring thread and check if the startInterval's default value is 5s instead of the original 30s.

- Description for the changelog

Fix the `StartInterval` default value of healthcheck to reflect the documented value of 5s.

- A picture of a cute animal (not mandatory but encouraged)

@j2walker j2walker changed the title Changed default value of the startInterval to 5s Changed default value of the startInterval to 5s Signed-off-by: Jack Walker <jack@bunkey.org> May 5, 2024
@j2walker j2walker changed the title Changed default value of the startInterval to 5s Signed-off-by: Jack Walker <jack@bunkey.org> Changed default value of the startInterval to 5s May 5, 2024
Signed-off-by: Jack Walker <90711509+j2walker@users.noreply.github.com>
@j2walker j2walker force-pushed the 47648-dameon-health-start-interval-default-value-fix branch from 03f6dea to e7fe35c Compare May 5, 2024 15:57
@vvoland vvoland added this to the 27.0.0 milestone May 7, 2024
@vvoland vvoland requested a review from cpuguy83 May 7, 2024 10:37
Copy link
Contributor

@vvoland vvoland left a comment

Choose a reason for hiding this comment

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

LGTM; except the small typo 😅

@@ -25,6 +25,9 @@ const (
// Also the time before the first probe.
defaultProbeInterval = 30 * time.Second

// Default interval between the container starting and the first probe run.
defaultStartInverval = 5 * time.Second
Copy link
Contributor

Choose a reason for hiding this comment

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

Small typo 😅

Suggested change
defaultStartInverval = 5 * time.Second
defaultStartInterval = 5 * time.Second

@@ -25,6 +25,9 @@ const (
// Also the time before the first probe.
defaultProbeInterval = 30 * time.Second

// Default interval between the container starting and the first probe run.
Copy link
Contributor

Choose a reason for hiding this comment

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

The comments seems not correct. Start interval is the time between health checks during the start period https://docs.docker.com/reference/dockerfile/#healthcheck

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.

Split Healthcheck start interval into its own default value as per docs
4 participants