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

introduce `workingdir` option for docker exec #35661

Merged
merged 2 commits into from Dec 6, 2017

Conversation

@ndeloof
Contributor

ndeloof commented Dec 1, 2017

Signed-off-by: Nicolas De Loof nicolas.deloof@gmail.com

- 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)

9788d27ee39ea4f56dcc3f784a8deede

introduce `workingdir` option for docker exec
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@ndeloof

This comment has been minimized.

Show comment
Hide comment
@ndeloof

ndeloof Dec 1, 2017

Contributor

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

Contributor

ndeloof commented Dec 1, 2017

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

@Phineas

Phineas approved these changes Dec 1, 2017

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Dec 1, 2017

Member

Design LGTM

Member

tonistiigi commented Dec 1, 2017

Design LGTM

@yongtang

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

@ndeloof ndeloof requested review from dnephin and vdemeester as code owners Dec 3, 2017

@dnephin

Thanks, couple comments about the test.

Show outdated Hide outdated integration/exec/main_test.go Outdated
Show outdated Hide outdated integration/exec/workingdir_test.go Outdated
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 4, 2017

Member

@dnephin PR was updated 👍

Member

thaJeztah commented Dec 4, 2017

@dnephin PR was updated 👍

@GordonTheTurtle GordonTheTurtle added dco/no and removed dco/no labels Dec 4, 2017

@moby moby deleted a comment from GordonTheTurtle Dec 4, 2017

@boaz1337

LGTM

@dnephin

dnephin approved these changes Dec 5, 2017

LGTM

Thanks!

Show outdated Hide outdated integration/container/exec_test.go Outdated
@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

test case to check « exec » works as expected
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
@ndeloof

This comment has been minimized.

Show comment
Hide comment
@ndeloof

ndeloof Dec 6, 2017

Contributor

commits squashed

Contributor

ndeloof commented Dec 6, 2017

commits squashed

@thaJeztah

LGTM, thanks!

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 6, 2017

Member

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 
Member

thaJeztah commented Dec 6, 2017

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

This comment has been minimized.

Show comment
Hide comment
@ndeloof

ndeloof Dec 6, 2017

Contributor

@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)

Contributor

ndeloof commented Dec 6, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 6, 2017

Member

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 👍

Member

thaJeztah commented Dec 6, 2017

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 👍

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

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 38243 has succeeded
Details
janky Jenkins build Docker-PRs 46986 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 7365 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18507 has succeeded
Details
z Jenkins build Docker-PRs-s390x 7187 has succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment