cli icon indicating copy to clipboard operation
cli copied to clipboard

switch to go module

Open crazy-max opened this issue 2 years ago • 10 comments

follow-up https://github.com/docker/cli/pull/3387#issuecomment-990904945

- What I did

Looks like we are ready to switch to go module for this repo as we are now compatible with Go's conventions (we are SemVer since v23.0.0).

- How I did it

- How to verify it

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

cc @thaJeztah @neersighted @corhere @vvoland @rumpl

crazy-max avatar Mar 24 '23 19:03 crazy-max

We'll have to rename all import paths (to include v23 (or likely v24 - see below) in the name, and rename the module to be the same.

Given that master is targeting v24, we probably need to tag the branch with a v24.0.0-alpha.0, otherwise it won't be importable as go modules produces a pseudo version based on the last tag from master (which I think was a v23)

thaJeztah avatar Mar 24 '23 21:03 thaJeztah

We'll have to rename all import paths (to include v23 (or likely v24 - see below) in the name, and rename the module to be the same.

That's exactly what I missed :see_no_evil:

crazy-max avatar Mar 24 '23 21:03 crazy-max

Codecov Report

Merging #4116 (9e3954b) into master (33961a7) will not change coverage. The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #4116   +/-   ##
=======================================
  Coverage   59.03%   59.03%           
=======================================
  Files         287      287           
  Lines       24767    24767           
=======================================
  Hits        14620    14620           
  Misses       9264     9264           
  Partials      883      883           

codecov-commenter avatar Mar 31 '23 23:03 codecov-commenter

@thaJeztah Should be good since https://github.com/docker/cli/releases/tag/v24.0.0-beta.1 :eyes:

crazy-max avatar Apr 01 '23 00:04 crazy-max

Thanks, looks hopeful (so far). FWIW; I was discussing this briefly in the maintainers call, and I think the initial response was to "Let's do both Moby and CLI at the same time". I'm personally partial to either (same time, or separately), but before we make the decision and plunge in, we should;

  • Do some testing / verification to see if there's other pain-points where we may be painting ourselves into a corner (more below)
  • Document the steps (and order / "sequence of steps") what to do when preparing a new release. The intent currently is to bump the "major" version fo every release, which means we will "rename the module" for every (non-patch) release

Reason this is important is that we have a very complex dependency tree due to circular dependencies;

  • Some of that we don't run into YET, because moby/moby is not a go module yet (so "has no version requirements for any of its dependencies")
    • ^^^ effectively, because docker/docker (moby) only has a vendor.mod (no go.mod), it currently "breaks the circular loop"
    • ^^^ once we have a go.mod in docker/docker (moby), that's no longer the case, and potentially "all hell breaks loose"
  • Some of that we may not run into ourselves but others may (thinking of docker compose, which depends on both docker/docker, docker/cli, AND docker/buildx)

A very quick (and limited) overview of this;

  • (1) docker/cli
    • (1.1) -> depends on docker/docker
      • (1.1.1) -> depends on buildkit (see 1.2)
      • (1.1.2) -> depends on containerd (see 1.4)
    • (1.2) -> depends on buildkit
      • (1.2.1) -> depends on docker/docker (back to 1.1)
      • (1.2.2) -> depends on docker/cli (back to 1)
      • (1.2.3) -> depends on containerd (see 1.4)
    • (1.3) -> depends on swarmkit
      • (1.2 A) -> depends on docker/docker (back to 1.1)
      • (1.2 B) -> depends on docker/cli (back to 1)
    • (1.4) -> depends on containerd
  • (2) docker/buildx
    • (2.1) -> depends on docker/cli (back to 1)
    • (2.2) -> depends on docker/docker (back to 1.1)
    • (2.3) -> depends on buildkit (back to 1.2)
    • (2.4) -> depends on containerd (see 1.4)
  • (3) docker/compose/v2
    • (3.1) -> depends on docker/cli (back to 1)
    • (3.2) -> depends on docker/buildx (back to 2)
    • (3.3) -> depends on buildkit (back to 1.2)
    • (3.4) -> depends on containerd (see 1.4)

Cut down circular dependencies

Perhaps we're able to remove docker/cli from some places.

  • I noticed that BuildKit only uses a very minimal set of packages, and perhaps those (or part of?) could be a separate module.
  • Possibly BuildKit's binaries (cmd/buildctl), which are probably not expected to be used as module, could be changed to a "sub" module
  • Possibly frontend/dockerfile could still be made its own module (and repo?)
  • Possibly builder/remotecontext/urlutil could still be made its own module (and repo?)

Looking at docker/cli's dependency on buildkit;

tree -d ./vendor/github.com/moby/buildkit
./vendor/github.com/moby/buildkit
├── frontend
│   └── dockerfile
│       └── dockerignore
└── util
    └── appcontext

Looking at buildkit, this is what it depends on;

tree -d ./vendor/github.com/docker/cli
./vendor/github.com/docker/cli
└── cli
    ├── config
    │   ├── configfile
    │   ├── credentials
    │   └── types
    └── connhelper
        └── commandconn

Slightly more from docker/docker (but those are expected, and "should be ok" for most of them);

./vendor/github.com/docker/docker
├── api
│   └── types
│       ├── blkiodev
│       ├── container
│       ├── events
│       ├── filters
│       ├── image
│       ├── mount
│       ├── network
│       ├── registry
│       ├── strslice
│       ├── swarm
│       │   └── runtime
│       ├── time
│       ├── versions
│       └── volume
├── client
├── errdefs
├── libnetwork
│   └── resolvconf
├── pkg
│   ├── archive
│   ├── chrootarchive
│   ├── homedir
│   ├── idtools
│   ├── ioutils
│   ├── longpath
│   ├── pools
│   ├── reexec
│   └── system
└── profiles
    └── seccomp

For buildx;

tree -d ./vendor/github.com/docker/cli
./vendor/github.com/docker/cli
├── cli
│   ├── command
│   ├── config
│   │   ├── configfile
│   │   ├── credentials
│   │   └── types
│   ├── connhelper
│   │   ├── commandconn
│   │   └── ssh
│   ├── context
│   │   ├── docker
│   │   └── store
│   ├── debug
│   ├── flags
│   ├── manifest
│   │   ├── store
│   │   └── types
│   ├── registry
│   │   └── client
│   ├── streams
│   ├── trust
│   └── version
├── cli-plugins
│   ├── manager
│   └── plugin
└── opts
tree -d ./vendor/github.com/docker/docker
./vendor/github.com/docker/docker
├── api
│   └── types
│       ├── blkiodev
│       ├── container
│       ├── events
│       ├── filters
│       ├── image
│       ├── mount
│       ├── network
│       ├── registry
│       ├── strslice
│       ├── swarm
│       │   └── runtime
│       ├── time
│       ├── versions
│       └── volume
├── builder
│   └── remotecontext
│       └── urlutil
├── client
├── errdefs
├── pkg
│   ├── archive
│   ├── homedir
│   ├── idtools
│   ├── ioutils
│   ├── jsonmessage
│   ├── longpath
│   ├── namesgenerator
│   ├── pools
│   ├── stdcopy
│   └── system
└── registry

From the above, I wonder if we need to create test-PRs that;

  1. update buildkit to use this PR (and perhaps even update moby to use that)
  2. update buildx to use this PR (and 1?)
  3. update compose to use this PR (and 2?)

thaJeztah avatar Apr 01 '23 14:04 thaJeztah

I moved this back to draft to prevent accidental merges, but let's continue the conversation, and draft a plan (I'm happy to see this part works at least; let's continue with the other parts to know / understand the full impact, and to see how we can move forward)

thaJeztah avatar Apr 01 '23 15:04 thaJeztah

Super important, would love to see this land. This is critical to the community, so that people can do "normal" building and debugging.

rfay avatar Feb 21 '24 18:02 rfay

@crazy-max - thanks for this great work. Would you mind rebasing it against current? It's super important to all of us.

rfay avatar Feb 21 '24 19:02 rfay

Just having a rebased PR would allow those of us who use normal tooling to debug through the cli and study its behavior. thanks!

rfay avatar Mar 01 '24 17:03 rfay

at this point, can one of the maintainers take over it to the new pr or forced update this pr? Thanks!

chenrui333 avatar Apr 10 '24 00:04 chenrui333