fix #35843 regression on health check workingdir#35845
Conversation
GordonTheTurtle
added
dco/no
Signed-off-by: Nicolas De Loof <[email protected]>
GordonTheTurtle
removed
the
dco/no
thaJeztah
left a comment
thaJeztah
left a comment
There was a problem hiding this comment.
change LGTM; perhaps a small test to catch regressions?
|
Also, thanks for the quick fix 👍 |
|
Sorry, won't have much time before Christmas vacations to investigate a test case (not sure how to wait for container to report Health logs) |
|
Something like this should be able to test the behaviour, but yes, we'll need a short timeout / loop to test the result docker run -dit \
--health-cmd="if [ \"\$PWD\" = \"/foo\" ]; then exit 0; else exit 1; fi;" \
--health-interval=0.5s \
--health-retries=1 \
--workdir=/foo \
busybox(edit: removed |
|
Had a quick try, and I have a test; will add it here |
|
Pushed a test; ping @dnephin @vdemeester @cpuguy83 PTAL |
There was a problem hiding this comment.
I'm not sure if an integration test is necessary here. Could this be tested with a unit test of run() instead?
Sleeping in a test case should generally be avoided. If this integration test is really necessary then this should use poll.WaitOn() to wait for the state, instead of sleep and inspect.
poll.WaitOn() will also make the test more reliable on loaded systems, because the timeout can be longer than 1s.
There was a problem hiding this comment.
I think an integration test is warranted here, because run and exec (used for the healthcheck) take different code paths.
Thanks for the poll.WaitOn(), I'll check that one out
|
Updated, PTAL 👍 |
Signed-off-by: Sebastiaan van Stijn <[email protected]>
|
LGTM |
8 participants
- What I did
Fix for #35843
- How I did it
Set recently introduced exec.WorkingDir to container's workingdir
- How to verify it
see #35843 description
- Description for the changelog
Fixed #35843
- A picture of a cute animal (not mandatory but encouraged)