Skip to content

Vendor swarmkit to 4429c763#35698

Merged
vieux merged 3 commits intomoby:masterfrom
anshulpundir:vndr
Dec 7, 2017
Merged

Vendor swarmkit to 4429c763#35698
vieux merged 3 commits intomoby:masterfrom
anshulpundir:vndr

Conversation

@anshulpundir
Copy link
Copy Markdown
Contributor

@anshulpundir anshulpundir commented Dec 5, 2017

- What I did
Vendor swarmkit to include moby/swarmkit@4429c76 (swarmkit master)

Comparison of changes moby/swarmkit@de950a7...4429c76

- How I did it

$ vi vendor.conf 
$ make BIND_DIR=. shell
$ vndr github.com/docker/swarmkit

@anshulpundir
Copy link
Copy Markdown
Contributor Author

ping @andrewhsu for review

@anshulpundir anshulpundir changed the title Vendor Swarmkit vendor to 4429c763 Vendor Swarmkit to 4429c763 Dec 5, 2017
@anshulpundir anshulpundir changed the title Vendor Swarmkit to 4429c763 Vendor swarmkit to 4429c763 Dec 5, 2017
@thaJeztah
Copy link
Copy Markdown
Member

Can you summarise the relevant changes? (perhaps link to those PR's where needed?)

@anshulpundir
Copy link
Copy Markdown
Contributor Author

anshulpundir commented Dec 5, 2017

The comparison of changes above has all the commits, which is a pretty good summary. LMK if more details are needed @thaJeztah

@andrewhsu
Copy link
Copy Markdown
Contributor

andrewhsu commented Dec 5, 2017

$ git log --oneline de950a7...4429c76 | grep 'Merge pull request' | awk '{print"docker/swarmkit" $5}' # list of swarmkit PRs in this vndr update

@anshulpundir
Copy link
Copy Markdown
Contributor Author

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>
@anshulpundir
Copy link
Copy Markdown
Contributor Author

@dperny for review

@dperny
Copy link
Copy Markdown
Contributor

dperny commented Dec 5, 2017

LGTM, but I am not a maintainer. The extra commit should fix a failing test.

@anshulpundir
Copy link
Copy Markdown
Contributor Author

#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

@nishanttotla
Copy link
Copy Markdown
Contributor

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.

@anshulpundir
Copy link
Copy Markdown
Contributor Author

anshulpundir commented Dec 6, 2017 via email

@GordonTheTurtle GordonTheTurtle added dco/no Automatically set by a bot when one of the commits lacks proper signature and removed rebuild/janky labels Dec 6, 2017
@GordonTheTurtle
Copy link
Copy Markdown

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ 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 -f

Amending updates the existing PR. You DO NOT need to open a new one.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@GordonTheTurtle GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Dec 6, 2017
Copy link
Copy Markdown
Contributor

@dperny dperny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM again, but I Am Not A Maintainer.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@tonistiigi
Copy link
Copy Markdown
Member

LGTM

Copy link
Copy Markdown
Contributor

@andrewhsu andrewhsu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vieux vieux merged commit a023a59 into moby:master Dec 7, 2017
@anshulpundir anshulpundir deleted the vndr branch December 7, 2017 01:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants