introduce `workingdir` option for docker exec by ndeloof · Pull Request #35661 · moby/moby

ndeloof · 2017-12-01T09:22:21Z

Signed-off-by: Nicolas De Loof [email protected]

- What I did

Implemented WorkingDir support for execs, as discussed on #8917

- How I did it

introduced WorkingDir in API and used for new process to be ran, falling back to container's WorkingDir if not set.

- How to verify it

Need to forge API call or use client package

- Description for the changelog

Introduce option to configure woking directory to run additional process in a container with exec

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

Signed-off-by: Nicolas De Loof <[email protected]>

ndeloof · 2017-12-01T09:48:21Z

I've ran make swagger-gen but don't get api/types/container/container_wait.go updated (which I do not expect) as reported by CI

tonistiigi · 2017-12-01T19:16:15Z

Design LGTM

yongtang

looks good to me, though can you add a test for it?

vdemeester

Design LGTM
/cc @thaJeztah @dnephin

dnephin

Thanks, couple comments about the test.

dnephin · 2017-12-04T15:20:32Z

exec is not a component (see https://github.com/moby/moby/tree/master/api/server/router). The component is container so this should go in integration/container.

ok, I can move this to container.

dnephin · 2017-12-04T15:21:36Z

We shouldn't write an integration test for option. How about just making this TestExec() and we can add a bunch of fields to this single test.

Do you mean you prefer a single, huge test to check all options and their correct behaviour ? Won't this make it harder to diagnose a build failure with a clear cause ?

It shouldn't be any harder if the failure messages are written correctly.

If there are options that conflict with each other, or that significantly change the behaviour of other options then multiple tests might be necessary, but I don't think that's the case here. Most of the options are independent, and we don't need to run multiple extremely slow tests to check each independent option.

right, but testing workdir require to set a custom command executed, while testing other options like env will require to run /bin/env, etc...

fair point. For those two cases I think env works for both, since PWD should be set. Is that right?

implemented as 57c5631

thaJeztah · 2017-12-04T20:51:45Z

@dnephin PR was updated 👍

boaz0

LGTM

dnephin

LGTM

Thanks!

dnephin · 2017-12-05T19:08:05Z

This cleanup is automatically handled by defer setupTest(t)() but I guess it doesn't hurt to have it, other than it might encourage others to add unnecessary cleanup to new tests.

thaJeztah

changes LGTM, but can you squash your commits (where needed)? Also while doing so, perhaps you can address @dnephin's nit https://github.com/moby/moby/pull/35661/files#r155046183

Then we can merge; if you can prepare a PR for the docker/cli it may just make it for 17.12

Signed-off-by: Nicolas De Loof <[email protected]>

ndeloof · 2017-12-06T06:43:53Z

commits squashed

thaJeztah

LGTM, thanks!

thaJeztah · 2017-12-06T07:18:16Z

oh! looks like there's an issue with the Swagger definition;

06:46:44 The result of hack/generate-swagger-api.sh differs
06:46:44 
06:46:44  M api/types/container/container_wait.go
06:46:44 
06:46:44 Please update api/swagger.yaml with any api changes, then

ndeloof · 2017-12-06T07:19:11Z

@thaJeztah yes, this happened on a regular basis on this PR, I can't explain why. Used to re-run janky build with rebuild/janky tag.
(it would help if this check also dump a diff, not just modified status)

thaJeztah · 2017-12-06T07:34:31Z

it would help if this check also dump a diff, not just modified status

Yes, definitely! I actually started building from this PR to check what the diff was, but I'm currently on super-slow-hotel-wifi, and just emptied my image cache before I left the office 😂

Let me trigger CI again to see if it resolves itself 👍

introduce workingdir option for docker exec

19f2749

Signed-off-by: Nicolas De Loof <[email protected]>

GordonTheTurtle added the status/0-triage label Dec 1, 2017

ndeloof mentioned this pull request Dec 1, 2017

Can't set docker exec working directory #8917

Closed

AkihiroSuda approved these changes Dec 1, 2017

View reviewed changes

AkihiroSuda added the rebuild/janky label Dec 1, 2017

GordonTheTurtle removed the rebuild/janky label Dec 1, 2017

Phineas approved these changes Dec 1, 2017

View reviewed changes

tonistiigi added status/1-design-review and removed status/0-triage labels Dec 1, 2017

yongtang reviewed Dec 1, 2017

View reviewed changes

ndeloof requested review from dnephin and vdemeester as code owners December 3, 2017 11:35

ndeloof force-pushed the 8917 branch 10 times, most recently from c259609 to 0ef331f Compare December 3, 2017 15:22

vdemeester reviewed Dec 4, 2017

View reviewed changes

vdemeester added status/2-code-review and removed status/1-design-review labels Dec 4, 2017

dnephin reviewed Dec 4, 2017

View reviewed changes

thaJeztah added impact/api impact/changelog labels Dec 4, 2017

thaJeztah added the impact/documentation label Dec 4, 2017

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Dec 4, 2017

ndeloof force-pushed the 8917 branch from cf13d22 to 57c5631 Compare December 4, 2017 21:16

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Dec 4, 2017

moby deleted a comment from GordonTheTurtle Dec 4, 2017

boaz0 approved these changes Dec 5, 2017

View reviewed changes

dnephin approved these changes Dec 5, 2017

View reviewed changes

thaJeztah reviewed Dec 6, 2017

View reviewed changes

test case to check « exec » works as expected

39d5ace

Signed-off-by: Nicolas De Loof <[email protected]>

ndeloof force-pushed the 8917 branch from 57c5631 to 39d5ace Compare December 6, 2017 06:43

ndeloof mentioned this pull request Dec 6, 2017

closes #8917 introduce —workdir option for docker exec docker/cli#732

Merged

thaJeztah approved these changes Dec 6, 2017

View reviewed changes

thaJeztah added the rebuild/janky label Dec 6, 2017

GordonTheTurtle removed the rebuild/janky label Dec 6, 2017

thaJeztah added status/4-merge rebuild/janky and removed status/2-code-review labels Dec 6, 2017

GordonTheTurtle removed the rebuild/janky label Dec 6, 2017

vdemeester merged commit 5e5fadb into moby:master Dec 6, 2017

thaJeztah mentioned this pull request Dec 6, 2017

Update API version-history for 1.35 #35724

Merged

thaJeztah mentioned this pull request Dec 20, 2017

Health check no longer uses the container's working directory #35843

Closed

This was referenced Jan 1, 2018

Fixes #68: failed healthchecks due to file path dockersamples/docker-swarm-visualizer#103

Merged

Since 17.12 docker CE - HEALTHCHECK no longer inherits WORKDIR (PWD==/) while in 17.09 HEALTHCHECK it was inherited from WORKDIR (PWD==WORKDIR). #35913

Closed

Conversation

ndeloof commented Dec 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ndeloof commented Dec 1, 2017

Uh oh!

tonistiigi commented Dec 1, 2017

Uh oh!

yongtang left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin Dec 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Dec 4, 2017

Uh oh!

boaz0 left a comment

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

ndeloof commented Dec 6, 2017

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Dec 6, 2017

Uh oh!

ndeloof commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Dec 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

ndeloof commented Dec 1, 2017 •

edited

Loading

dnephin Dec 4, 2017 •

edited

Loading

ndeloof commented Dec 6, 2017 •

edited

Loading