Conversation
|
ping @andrewhsu for review |
|
Can you summarise the relevant changes? (perhaps link to those PR's where needed?) |
|
The comparison of changes above has all the commits, which is a pretty good summary. LMK if more details are needed @thaJeztah |
|
BTW, I see some tests failing in janky because of the changes being pulled in. Looks like the tests will have to be modified/fixed. |
Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>
…th swarmkit#b187b24 service deletions/scale-downs don't directly remove the container. Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>
|
@dperny for review |
|
LGTM, but I am not a maintainer. The extra commit should fix a failing test. |
|
#34967 should be fixed by moby/swarmkit#2461 / moby/swarmkit#2446, but I don't think this was tested. I don't know if it fixes #34574 (also not tested after moby/swarmkit#2461 / moby/swarmkit#2446 went in) @thaJeztah |
|
We only have unit tests for moby/swarmkit#2461 / moby/swarmkit#2446, but it would be advisable to add end to end testing as well. I do believe that #34574 is fixed by those PRs. |
|
Agreed. We can close these issues after adding the e2e tests.
…On Tue, Dec 5, 2017 at 8:46 PM Nishant Totla ***@***.***> wrote:
We only have unit tests for moby/swarmkit#2461
<moby/swarmkit#2461> / moby/swarmkit#2446
<moby/swarmkit#2446>, but it would be advisable
to add end to end testing as well. I do believe that #34574
<#34574> is fixed by those PRs.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#35698 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AM7HYXh5dFr3FCNmY7isoeca9N3L7bxkks5s9hwYgaJpZM4Q1jov>
.
|
|
Please sign your commits following these rules: $ git clone -b "vndr" git@github.com:anshulpundir/moby.git somewhere
$ cd somewhere
$ git rebase -i HEAD~842354331352
editor opens
change each 'pick' to 'edit'
save the file and quit
$ git commit --amend -s --no-edit
$ git rebase --continue # and repeat the amend for each commit
$ git push -fAmending updates the existing PR. You DO NOT need to open a new one. |
dperny
left a comment
There was a problem hiding this comment.
LGTM again, but I Am Not A Maintainer.
daemon/cluster/swarm.go
Outdated
There was a problem hiding this comment.
At first I thought this might ought to go in daemon/cluster/convert/swarm.go, but after stewing on it for a few minutes, I think this is an equally good place. Doesn't mix validation and conversion logic. LGTM.
There was a problem hiding this comment.
I change my mind lol this actually does need to be changed to wrap this error in one of the errors in daemon/cluster/errors.go. If you just return a raw error, it'll probably end up being an 500 Internal at the HTTP api boundary.
Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>
|
LGTM |
- What I did
Vendor swarmkit to include moby/swarmkit@4429c76 (swarmkit master)
Comparison of changes moby/swarmkit@de950a7...4429c76
- How I did it