Skip to content
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

Remove service update name flag #26988

Merged
merged 1 commit into from Oct 27, 2016

Conversation

@thaJeztah
Copy link
Member

commented Sep 28, 2016

The --name flag was inadvertently added to docker service update, but is not supported, as it has various side-effects (e.g., existing tasks are not renamed).

This removes the flag from the service update command and API documentation.

fixes #25483

Note for reviewing

I opened this PR for discussion, but as it is a change in both the CLI and the API, I'm not sure what the best way forward is. I don't know if this can be seen as a regular bugfix, or if we need to start a deprecation traject.

ping @stevvooe @aluzzardi PTAL

@@ -18,6 +18,7 @@ import (

var (
errNetworkUpdateNotSupported = errors.New("changing network in service is not supported")
errRenameNotSupported = errors.New("service renaming is not supported")

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann Sep 29, 2016

Contributor

nit: I prefer "renaming services is not supported"

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Sep 29, 2016

Author Member

yes, that may be better (I based this one on "service mode change is not allowed" below)

@@ -349,6 +350,11 @@ func (s *Server) UpdateService(ctx context.Context, request *api.UpdateServiceRe
if request.Spec != nil && reflect.TypeOf(service.Spec.Mode) != reflect.TypeOf(request.Spec.Mode) {
return errModeChangeNotAllowed
}

if request.Spec != nil && service.Spec.Annotations.Name != request.Spec.Annotations.Name {

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann Sep 29, 2016

Contributor

The request.Spec != nil check is not necessary, and we can also remove the others above. validateServiceSpec ensures that it is not nil.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Sep 29, 2016

Author Member

Nice! Let me open a separate PR for the others in the SwarmKit repo

This comment has been minimized.

Copy link
@thaJeztah
@@ -4785,7 +4785,6 @@ image](#create-an-image) section for more details.
Content-Type: application/json
{
"Name": "top",

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann Sep 29, 2016

Contributor

Name is still part of the service update API - in fact, I believe that omitting it will trigger the service renaming check that you added here. It is just imperative that the client not change the name from its previous value.

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Sep 29, 2016

Author Member

I tried doing an API request that omitted the "Name", and I think it worked, but I'll double check with the current code (as I made some modifications since, and possibly I had request.Spec.Annotations.Name != nil && ... in the previous version).

This comment has been minimized.

Copy link
@thaJeztah

thaJeztah Sep 29, 2016

Author Member

And, yes, I was wondering if there should be a separate ServiceUpdate API type (without the "Name"). IIUC, the overall design is that Update request should only have to send those parts of the Service / Spec that need to be modified (only a "diff"), correct?

This comment has been minimized.

Copy link
@aaronlehmann

aaronlehmann Sep 29, 2016

Contributor

No, the model is that the client should send the entire spec on a update.

See docker/swarmkit#1394 (comment)

@thaJeztah

This comment has been minimized.

Copy link
Member Author

commented Sep 29, 2016

We discussed this in the maintainers meeting, and this is a bugfix, so we can go ahead with this change.

@tonistiigi can you add the missing info on the API change? ❤️

@stevvooe

This comment has been minimized.

Copy link
Contributor

commented Oct 3, 2016

Design LGTM

The messaging is going to be complex here.

@tonistiigi

This comment has been minimized.

Copy link
Member

commented Oct 3, 2016

@thaJeztah As @aaronlehmann already commented, Docker side API should not change with this. We already send full object, even when no cli changes and we should continue to do so. We can add documentation that even though this field is part of the object you may not change that in a later update. We already have couple of restrictions like this.

@thaJeztah thaJeztah force-pushed the thaJeztah:remove-service-update-name-flag branch 3 times, most recently from 3f3587b to 0cea58b Oct 6, 2016

@thaJeztah

This comment has been minimized.

Copy link
Member Author

commented Oct 6, 2016

@aaronlehmann @tonistiigi updated; added the "name" field back to the API, and updated the description to explain that updating the value is not allowed

@aaronlehmann

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2016

LGTM

@tonistiigi

This comment has been minimized.

Copy link
Member

commented Oct 6, 2016

LGTM

1 similar comment
@vieux

This comment has been minimized.

Copy link
Collaborator

commented Oct 6, 2016

LGTM

@vieux

This comment has been minimized.

Copy link
Collaborator

commented Oct 6, 2016

@thaJeztah could you open the vendor/src/github.com/docker/swarmkit/manager/controlapi/service.go changes in swarmkit ?

@thaJeztah thaJeztah force-pushed the thaJeztah:remove-service-update-name-flag branch from 0cea58b to 20529f6 Oct 10, 2016

@thaJeztah

This comment has been minimized.

Copy link
Member Author

commented Oct 10, 2016

yongtang added a commit to yongtang/docker that referenced this pull request Oct 26, 2016
Revendor swarmkit to 0ec7c6ee4b3185ec4e3d6bd65f8f5542b1761421
This fix revendor swarmkit to 0ec7c6ee4b3185ec4e3d6bd65f8f5542b1761421.

Related docker PR and issues:
(moby#27567)
(moby#25437)
(moby#26988)
(moby#25644)

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
yongtang added a commit to yongtang/docker that referenced this pull request Oct 26, 2016
Revendor swarmkit to 0ec7c6ee4b3185ec4e3d6bd65f8f5542b1761421
This fix revendor swarmkit to 0ec7c6ee4b3185ec4e3d6bd65f8f5542b1761421.

Related docker PR and issues:
(moby#27567)
(moby#25437)
(moby#26988)
(moby#25644)

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

@thaJeztah thaJeztah added this to the 1.13.0 milestone Oct 26, 2016

@thaJeztah thaJeztah force-pushed the thaJeztah:remove-service-update-name-flag branch from 20529f6 to f49d1e7 Oct 26, 2016

@xiaods

This comment has been minimized.

Copy link
Contributor

commented Oct 27, 2016

any udpate?

Remove --name flag from service update
The --name flag was inadvertently added to
docker service update, but is not supported,
as it has various side-effects (e.g., existing
tasks are not renamed).

This removes the flag from the service update
command.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

@thaJeztah thaJeztah force-pushed the thaJeztah:remove-service-update-name-flag branch from f49d1e7 to 047e44e Oct 27, 2016

@thaJeztah

This comment has been minimized.

Copy link
Member Author

commented Oct 27, 2016

@thaJeztah thaJeztah changed the title [WIP] Remove service update name flag Remove service update name flag Oct 27, 2016

@aluzzardi

This comment has been minimized.

Copy link
Member

commented Oct 27, 2016

LGTM

@vdemeester

This comment has been minimized.

Copy link
Member

commented Oct 27, 2016

docs LGTM 🐮

@thaJeztah

This comment has been minimized.

Copy link
Member Author

commented Oct 27, 2016

author LGTM

@thaJeztah thaJeztah merged commit 3a3a87b into moby:master Oct 27, 2016

4 checks passed

docker/dco-signed All commits signed
Details
experimental Jenkins build Docker-PRs-experimental 25554 has succeeded
Details
janky Jenkins build Docker-PRs 34153 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 5033 has succeeded
Details

@thaJeztah thaJeztah deleted the thaJeztah:remove-service-update-name-flag branch Oct 27, 2016

dnephin pushed a commit to dnephin/docker that referenced this pull request Apr 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.