Remove service update name flag#26988
Conversation
There was a problem hiding this comment.
nit: I prefer "renaming services is not supported"
There was a problem hiding this comment.
yes, that may be better (I based this one on "service mode change is not allowed" below)
There was a problem hiding this comment.
The request.Spec != nil check is not necessary, and we can also remove the others above. validateServiceSpec ensures that it is not nil.
There was a problem hiding this comment.
Nice! Let me open a separate PR for the others in the SwarmKit repo
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
No, the model is that the client should send the entire spec on a update.
|
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? ❤️ |
|
Design LGTM The messaging is going to be complex here. |
|
@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
force-pushed
the
remove-service-update-name-flag
branch
3 times, most recently
from
3f3587b to
0cea58b
Compare
|
@aaronlehmann @tonistiigi updated; added the "name" field back to the API, and updated the description to explain that updating the value is not allowed |
|
LGTM |
|
LGTM |
1 similar comment
|
LGTM |
|
@thaJeztah could you open the |
thaJeztah
force-pushed
the
remove-service-update-name-flag
branch
from
0cea58b to
20529f6
Compare
|
opened moby/swarmkit#1606 |
This fix revendor swarmkit to 0ec7c6e. Related docker PR and issues: (moby#27567) (moby#25437) (moby#26988) (moby#25644) Signed-off-by: Yong Tang <[email protected]>
thaJeztah
force-pushed
the
remove-service-update-name-flag
branch
from
20529f6 to
f49d1e7
Compare
|
any udpate? |
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 <[email protected]>
thaJeztah
force-pushed
the
remove-service-update-name-flag
branch
from
f49d1e7 to
047e44e
Compare
thaJeztah
changed the title
|
LGTM |
|
docs LGTM 🐮 |
|
author LGTM |
…me-flag Remove service update name flag
9 participants
The
--nameflag was inadvertently added todocker 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