Skip to content

Update import path from docker/docker to moby/moby#35361

Closed
cpuguy83 wants to merge 1 commit intomoby:masterfrom
cpuguy83:migrate_to_moby_import_paths
Closed

Update import path from docker/docker to moby/moby#35361
cpuguy83 wants to merge 1 commit intomoby:masterfrom
cpuguy83:migrate_to_moby_import_paths

Conversation

@cpuguy83
Copy link
Copy Markdown
Member

@cpuguy83 cpuguy83 commented Nov 1, 2017

This is part of the effort to migrate from docker/docker to moby/moby.

This will have an impact on people who consume this repo. We should have a communication plan for this change (moby blog post?).

If you can think of other todo's outside of communication, please comment.

  • - Make vendor forks for libnetwork and swarmkit
  • - Update the real libnetwork and swarmkit after this is merged

@cpuguy83 cpuguy83 changed the title [WIP] Update import path from docker/docker to moby/moby Update import path from docker/docker to moby/moby Nov 1, 2017
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Gogland did this automatically for me (I think it runs goimports) but some imports may need to be re-sorted after the rename

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

go fmt'd everything on the last push.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Awesome, thanks!

Copy link
Copy Markdown
Contributor

@tiborvass tiborvass Nov 2, 2017

Choose a reason for hiding this comment

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

Yes, we need to run gofmt on all the files that got modified.

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.

gofmt should be sufficient in this case.

@cpuguy83 cpuguy83 force-pushed the migrate_to_moby_import_paths branch from f0e10d5 to a71ca1a Compare November 1, 2017 20:49
@thaJeztah
Copy link
Copy Markdown
Member

20:55:12 # github.com/moby/moby/daemon/cluster/executor/container
20:55:12 daemon/cluster/executor/container/container_test.go:6:2: cannot find package "github.com/docker/docker/api/types/container" in any of:
20:55:12 	/go/src/github.com/moby/moby/vendor/github.com/docker/docker/api/types/container (vendor tree)

@thaJeztah
Copy link
Copy Markdown
Member

Not sure what happened on Janky though; related to the autogen changes?

20:52:27 Successfully built ebffcb9dca5e
20:52:27 fatal: Not a git repository (or any of the parent directories): .git
20:52:27 Build step 'Execute shell' marked build as failure

@cpuguy83
Copy link
Copy Markdown
Member Author

cpuguy83 commented Nov 1, 2017 via email

@cpuguy83
Copy link
Copy Markdown
Member Author

cpuguy83 commented Nov 1, 2017 via email

@cpuguy83 cpuguy83 force-pushed the migrate_to_moby_import_paths branch from a71ca1a to 379ad24 Compare November 2, 2017 14:26
This is part of the effort to migrate from docker/docker to moby/moby.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>
@cpuguy83 cpuguy83 force-pushed the migrate_to_moby_import_paths branch from 379ad24 to a7e793e Compare November 2, 2017 17:02
@cpuguy83
Copy link
Copy Markdown
Member Author

cpuguy83 commented Nov 2, 2017

Vendor forks done.

@cyphar
Copy link
Copy Markdown
Contributor

cyphar commented Nov 14, 2017

This is going to either break or cause annoyance to every project that vendors github.com/docker/docker, which is quite worrying. If you look at how bad the transition was for logrus (just one small package) I don't imagine that doubling the size of vendor/ is going to be a good idea (since github.com/docker/docker is quite massive).

@cpuguy83
Copy link
Copy Markdown
Member Author

@cyphar There are a couple of places where this is especially difficult... specifically api/types/ and client/. The rest of the repo is not generally considered stable from a package API standpoint (and I'm sure we've done a poor job in keeping the types and client package API's totally stable).

But yes, definitely aware this is going to cause problems. I think with enough for-warning it won't be so bad, however it is difficult to prepare a repo for this change since someone can't just gradually make this change or even switch to new imports now rather than after the change (e.g. like they could for when grpc changed repos).

@stevvooe
Copy link
Copy Markdown
Contributor

stevvooe commented Jan 8, 2018

@cyphar Just a reminder that the logrus change was problematic due to casing. One could not have Sirupsen and sirupsen in the same project, making the change impossible to do gracefully. The fix here will just be a package rewrite.

@vdemeester
Copy link
Copy Markdown
Member

@cpuguy83 what is the status for this one ? (you have soooo much conflicts 🤗)

@vdemeester
Copy link
Copy Markdown
Member

Given the PR on canonical import : #36194 should we close this one for now ?

@cpuguy83
Copy link
Copy Markdown
Member Author

Yeah, fine closing this now.

@cpuguy83 cpuguy83 closed this Feb 16, 2018
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.

8 participants