-
Notifications
You must be signed in to change notification settings - Fork 70
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
healthcheck: add support for --start-interval
#280
healthcheck: add support for --start-interval
#280
Conversation
Hi @flouthoc. Thanks for your PR. I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
477f658
to
3a90fde
Compare
If this gets merged I'll open a followup PR in buildah and vendor new imagebuilder there. |
Then this should probably include a commit to bump the release version. |
@nalind Did you mean a version bump for imagebuilder itself ? |
Yes, unless there's some reason not to. |
Other than @nalind 's comments, LGTM |
@nalind @TomSweeneyRedHat done. |
53d844e
to
d261b22
Compare
@@ -7,4 +7,6 @@ HEALTHCHECK --interval=5s --timeout=3s --retries=3 \ | |||
HEALTHCHECK CMD | |||
HEALTHCHECK CMD a b | |||
HEALTHCHECK --timeout=3s CMD ["foo"] | |||
HEALTHCHECK --start-interval=5s --interval=5s --timeout=3s --retries=3 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one could use a "--start-period" flag, since it's exercising the parser, and I don't see anything else in this directory checking that the flag is recognized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
LGTM with one nit, which isn't a blocker. |
Signed-off-by: flouthoc <flouthoc.git@gmail.com>
d261b22
to
9556267
Compare
@nalind Is it that CI is using older |
It's certainly plausible. My access is limited, so I can't actually read the error logs. |
@nalind In error logs at least it is saying that docker cannot parse
|
Should i try this ? https://docs.travis-ci.com/user/docker/#installing-a-newer-docker-version in a different commit. |
That's consistent with the version information at https://docs.travis-ci.com/user/reference/jammy/.
Any reason not to? |
@nalind A different test is failing i.e |
@nalind This fixes error for me and is compatible with newer buildkit, but I am not sure why this additional diff --git a/dockerclient/client.go b/dockerclient/client.go
index 1e8c83f..1236d66 100644
--- a/dockerclient/client.go
+++ b/dockerclient/client.go
@@ -1058,7 +1058,7 @@ func (e *ClientExecutor) CopyContainer(container *docker.Container, excludes []s
}
chmod = func(h *tar.Header, r io.Reader) (data []byte, update bool, skip bool, err error) {
mode := h.Mode &^ 0o777
- mode |= parsed & 0o777
+ mode |= parsed
h.Mode = mode
return nil, false, false, nil |
The intent there was to avoid cases where bits that attempt to change the file type were added (the bits for block and character devices fit within |
Add support for `--start-interval` which is present in newer version of docker Ref: https://docs.docker.com/reference/dockerfile/#healthcheck Signed-off-by: flouthoc <flouthoc.git@gmail.com>
add healthcheck to conformance tests Signed-off-by: flouthoc <flouthoc.git@gmail.com>
Signed-off-by: flouthoc <flouthoc.git@gmail.com>
Signed-off-by: flouthoc <flouthoc.git@gmail.com>
Update version information to mark this as v1.2.8 Signed-off-by: flouthoc <flouthoc.git@gmail.com>
e541458
to
2eccd9a
Compare
Thanks this worked, I think CI should pass now. |
Oof, I meant chmod, but anyway, LGTM. |
Update version information to mark this as v1.2.9-dev. Signed-off-by: flouthoc <flouthoc.git@gmail.com>
2eccd9a
to
2eb02af
Compare
Okay I think all the test passed last time except one which failed and I think it was a size diff fluke in conformance test, I think it should pass in this re-push. |
@nalind @rhatdan @TomSweeneyRedHat PTAL , finally CI is green. |
LGTM and happy green test buttons |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
@flouthoc: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
@nalind @TomSweeneyRedHat Could we cut a new imagebuilder version whenever possible, thanks. |
Add support for
--start-interval
which is present in newer version of dockerRef: https://docs.docker.com/reference/dockerfile/#healthcheck
Needed by: containers/podman#21701