Conversation
|
ping @vieux @aaronlehmann ptal |
Current coverage is 54.66% (diff: 0.00%)@@ master #1606 diff @@
==========================================
Files 84 84
Lines 14174 14176 +2
Methods 0 0
Messages 0 0
Branches 0 0
==========================================
- Hits 7776 7750 -26
- Misses 5381 5396 +15
- Partials 1017 1030 +13
|
|
LGTM |
manager/controlapi/service.go
Outdated
|
|
||
| var ( | ||
| errNetworkUpdateNotSupported = errors.New("changing network in service is not supported") | ||
| errRenameNotSupported = errors.New("renaming service is not supported") |
There was a problem hiding this comment.
nit: service should be plural: renaming services is not supported
There was a problem hiding this comment.
oh, makes sense let me update
thaJeztah
force-pushed
the
disallow-rename
branch
from
5e6a629 to
34c8908
Compare
| } | ||
|
|
||
| if service.Spec.Annotations.Name != request.Spec.Annotations.Name { | ||
| return errRenameNotSupported |
There was a problem hiding this comment.
It just occurred to me that it might be helpful to show the old and new name of the service, in case a user (especially an API user) encounters this unexpectedly. WDYT? I don't think it's very important though, so feel free to ignore this suggestion.
There was a problem hiding this comment.
This should also be a proper grpc error.
|
LGTM |
|
I discussed with @aaronlehmann and it may be a nice addition to show the names of the service for easier tracking of errors. Also, it wouldn't hurt to add a unit-test, so I'll have a look at those. I don't think there's a hurry to get this merged, so I'll ping when I made those changes |
|
@thaJeztah: Are you still planning to make these changes? |
|
@thaJeztah: Are you still planning to make these changes? |
Updating a services' name has has various side-effects (e.g., existing tasks are not renamed), and was never intended to be supported. This produces an error if the service is renamed. Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
disallow-rename
branch
from
34c8908 to
03860b2
Compare
|
LGTM There are more changes to make the error handling correct that are outside of the scope of this PR. |
|
@aaronlehmann just discussed with @stevvooe, and I'm ok with getting this merged as-is, so that other fixes can be taken care of in a follow up. I'll open an issue for that |
|
Just want to confirm we're not going to fix this to return a gRPC error before merging? That seems like a very simple thing to fix. |
|
I see that there are other cases where this handler does not return gRPC errors, so I guess that's why... |
|
@aaronlehmann yup 😄 |
|
LGTM |
8 participants
Updating a services' name has has various side-effects (e.g., existing tasks are not renamed), and was never intended to be supported.
This produces an error if the service is renamed.
relates to moby/moby#26988