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

Add `--read-only` for `service create` and `service update` #30162

Merged
merged 2 commits into from Jan 31, 2017

Conversation

Projects
None yet
7 participants
@yongtang
Member

yongtang commented Jan 14, 2017

- What I did

This fix tries to address the issue raised in #29972 where it was not possible to specify --read-only for docker service create and docker service update, in order to have the container's root file system to be read only.

- How I did it

This fix adds --read-only and update the ReadonlyRootfs in HostConfig through service create and service update.

Related docs has been updated.

- How to verify it

Integration test has been added.

- Description for the changelog

Add --read-only for service create and service update

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

baby-animals-cat-cats-cute-kitten-kittens-desktop-image

This fix fixes #29972.

Related SwarmKit PR is docker/swarmkit#1872

Signed-off-by: Yong Tang yong.tang.github@outlook.com

@thaJeztah

design looks good to me; left some general thoughts on the tests

Show outdated Hide outdated integration-cli/docker_cli_swarm_test.go Outdated
Show outdated Hide outdated integration-cli/docker_cli_swarm_test.go Outdated
Show outdated Hide outdated integration-cli/docker_cli_swarm_test.go Outdated
Show outdated Hide outdated cli/command/service/opts.go Outdated
@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 27, 2017

Contributor

@yongtang there are some comments from @thaJeztah
Thanks!

Contributor

LK4D4 commented Jan 27, 2017

@yongtang there are some comments from @thaJeztah
Thanks!

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 27, 2017

Member

@LK4D4 Thanks for the reminder. docker/swarmkit#1872 was merged yesterday. I will work on updating this PR shortly.

Member

yongtang commented Jan 27, 2017

@LK4D4 Thanks for the reminder. docker/swarmkit#1872 was merged yesterday. I will work on updating this PR shortly.

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 28, 2017

Member

@thaJeztah Thanks for the review. The PR has been updated. And some of the tests have been changed to unit tests. Please take a look and let me know if there are any issues.

Member

yongtang commented Jan 28, 2017

@thaJeztah Thanks for the review. The PR has been updated. And some of the tests have been changed to unit tests. Please take a look and let me know if there are any issues.

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 30, 2017

Contributor

@yongtang I'm not sure, but failures look real enough

Contributor

LK4D4 commented Jan 30, 2017

@yongtang I'm not sure, but failures look real enough

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 30, 2017

Member

@LK4D4 Ah the failure is related to the changes in SwarmKit about explicitly disallow network pluginv1 creation in swarm mode (See discussion in docker/swarmkit#1899, docker/swarmkit#1894, and #30332)

The failure is being addressed in PR #30548 now. Please take a look.

Member

yongtang commented Jan 30, 2017

@LK4D4 Ah the failure is related to the changes in SwarmKit about explicitly disallow network pluginv1 creation in swarm mode (See discussion in docker/swarmkit#1899, docker/swarmkit#1894, and #30332)

The failure is being addressed in PR #30548 now. Please take a look.

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 30, 2017

Contributor

@yongtang cool, thanks!

Contributor

LK4D4 commented Jan 30, 2017

@yongtang cool, thanks!

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 30, 2017

Contributor

@yongtang just merged it.

Contributor

LK4D4 commented Jan 30, 2017

@yongtang just merged it.

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 30, 2017

Member

Thanks @LK4D4! This PR has been updated as well now.

Member

yongtang commented Jan 30, 2017

Thanks @LK4D4! This PR has been updated as well now.

@thaJeztah

changes LGTM perhaps you can also update the completion scripts for bash and zsh as part of this PR? I think bash change should go here; https://github.com/docker/docker/blob/master/contrib/completion/bash/docker#L2833 (but double check 😄)

yongtang added some commits Jan 14, 2017

Add `--read-only` for `service create` and `service update`
This fix tries to address the issue raised in 29972 where
it was not possible to specify `--read-only` for `docker service create`
and `docker service update`, in order to have the container's root file
system to be read only.

This fix adds `--read-only` and update the `ReadonlyRootfs` in `HostConfig`
through `service create` and `service update`.

Related docs has been updated.

Integration test has been added.

This fix fixes 29972.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Update bash and zsh completion for `service create/update --read-only`
This commit updates bash and zsh completion for flag `--read-only`
in `service create/update`.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Jan 30, 2017

Member

Thanks @thaJeztah. The PR has been updated with related bash and zsh completion added.

Member

yongtang commented Jan 30, 2017

Thanks @thaJeztah. The PR has been updated with related bash and zsh completion added.

@thaJeztah

LGTM, thanks!

@LK4D4

This comment has been minimized.

Show comment
Hide comment
@LK4D4

LK4D4 Jan 31, 2017

Contributor

LGTM

Contributor

LK4D4 commented Jan 31, 2017

LGTM

@LK4D4 LK4D4 merged commit 1d2f5de into moby:master Jan 31, 2017

4 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 30122 has succeeded
Details
janky Jenkins build Docker-PRs 38735 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 9763 has succeeded
Details

@GordonTheTurtle GordonTheTurtle added this to the 1.14.0 milestone Jan 31, 2017

@yongtang yongtang deleted the yongtang:29972-service-read-only branch Jan 31, 2017

@vieux

This comment has been minimized.

Show comment
Hide comment
@vieux

vieux Feb 18, 2017

Collaborator

@yongtang can you please open a PR to update docs/api/version-history.md

Collaborator

vieux commented Feb 18, 2017

@yongtang can you please open a PR to update docs/api/version-history.md

@yongtang

This comment has been minimized.

Show comment
Hide comment
@yongtang

yongtang Feb 18, 2017

Member

Thanks @vieux for the reminder. The PR #31145 has been opened.

Member

yongtang commented Feb 18, 2017

Thanks @vieux for the reminder. The PR #31145 has been opened.

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017

Merge pull request moby#30162 from yongtang/29972-service-read-only
Add `--read-only` for `service create` and `service update`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment