Skip to content

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jul 21, 2025

@thaJeztah thaJeztah requested a review from a team as a code owner July 21, 2025 18:51
@thaJeztah thaJeztah requested review from glours and ndeloof July 21, 2025 18:51
@thaJeztah thaJeztah marked this pull request as draft July 21, 2025 18:51
@thaJeztah thaJeztah changed the title Bump moby cli WIP: migrate to moby modules Jul 21, 2025
@thaJeztah thaJeztah force-pushed the bump_moby_cli branch 2 times, most recently from a5b199b to 73bb946 Compare July 21, 2025 18:55
@ndeloof
Copy link
Contributor

ndeloof commented Jul 21, 2025

Nice to see moby API as a dedicated module, so we don't get transitive dependencies from engine inside Compose ! ❤️

@@ -0,0 +1,89 @@
// Package urlutil provides helper function to check if a given build-context
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this fork temporarily; didn't realise compose itself also uses it (not only through cli); the CLI fork is "internal", so probably needs to made public, or we keep a fork here 😞

@thaJeztah
Copy link
Member Author

Remaining uses of docker/docker;

tree -d vendor/github.com/docker/docker
vendor/github.com/docker/docker
├── internal
│   └── lazyregexp
├── pkg
│   ├── jsonmessage
│   ├── namesgenerator
│   ├── pidfile
│   ├── process
│   ├── progress
│   ├── stdcopy
│   ├── streamformatter
│   └── stringid
└── registry

13 directories

@thaJeztah
Copy link
Member Author

Build failure looks like it may be an incompatible change in buildx itself?

 > [build 1/1] RUN --mount=type=bind,target=.     --mount=type=cache,target=/root/.cache     --mount=type=cache,target=/go/pkg/mod     --mount=type=bind,from=osxcross,src=/osxsdk,target=/xx-sdk     xx-go --wrap &&     if [ "$(xx-info os)" == "darwin" ]; then export CGO_ENABLED=1; fi &&     make build GO_BUILDTAGS="e2e" DESTDIR=/out &&     xx-verify --static /out/docker-compose:
1.027 GO111MODULE=on go build  -trimpath -tags "e2e" -ldflags "-w -X github.com/docker/compose/v2/internal.Version=fa50b46" -o "/out/docker-compose" ./cmd
38.90 # github.com/docker/buildx/build
38.90 /go/pkg/mod/github.com/tha!jeztah/buildx@v0.2.1-0.20250721183044-80b40a761230/build/provenance.go:135:16: pred1.ConvertToSLSA02 undefined (type *"github.com/moby/buildkit/solver/llbsolver/provenance/types".ProvenancePredicateSLSA1 has no field or method ConvertToSLSA02)
67.83 make: *** [Makefile:61: build] Error 1
38.90 /go/pkg/mod/github.com/tha!jeztah/buildx@v0.2.1-0.20250721183044-80b40a761230/build/provenance.go:135:16: pred1.ConvertToSLSA02 undefined (type 

@thaJeztah
Copy link
Member Author

OK, it's because buildx depends on an unreleased version of BuildKit, so go modules considers it to be older than the latest release and won't update; https://github.com/docker/buildx/blob/3f4bf829d8c5b92a8f670609417d9aa4c0b6be98/go.mod#L32

github.com/moby/buildkit v0.23.0-rc1.0.20250618182037-9b91d20367db // master

@thaJeztah thaJeztah force-pushed the bump_moby_cli branch 2 times, most recently from 83ca48f to 13827c4 Compare July 21, 2025 19:20
@ndeloof
Copy link
Contributor

ndeloof commented Jul 22, 2025

  • jsonmessage
    this one should be part of the API, don't you think ?

@thaJeztah
Copy link
Member Author

Yes, or at least the type; the utilities could be "client" - those packages are messy.

Also looking at things that are in the api, but shouldn't (api/types/plugins/logdriver)

@thaJeztah
Copy link
Member Author

Also included a (WIP) branch for docker/cli#6202

Before/After;

Details

Before:

tree -d vendor/github.com/docker/cli
vendor/github.com/docker/cli
├── cli
│   ├── command
│   │   ├── completion
│   │   ├── container
│   │   ├── formatter
│   │   │   └── tabwriter
│   │   ├── image
│   │   │   └── build
│   │   │       └── internal
│   │   │           ├── git
│   │   │           └── urlutil
│   │   └── inspect
│   ├── compose
│   │   ├── interpolation
│   │   ├── loader
│   │   ├── schema
│   │   │   └── data
│   │   ├── template
│   │   └── types
│   ├── config
│   │   ├── configfile
│   │   ├── credentials
│   │   ├── memorystore
│   │   └── types
│   ├── connhelper
│   │   ├── commandconn
│   │   ├── internal
│   │   │   └── syntax
│   │   └── ssh
│   ├── context
│   │   ├── docker
│   │   └── store
│   ├── debug
│   ├── flags
│   ├── hints
│   ├── streams
│   ├── trust
│   └── version
├── cli-plugins
│   ├── hooks
│   ├── manager
│   ├── metadata
│   ├── plugin
│   └── socket
├── internal
│   ├── jsonstream
│   ├── lazyregexp
│   ├── prompt
│   └── tui
├── opts
│   └── swarmopts
├── pkg
│   └── kvfile
└── templates

55 directories

After:

tree -d vendor/github.com/docker/cli
vendor/github.com/docker/cli
├── cli
│   ├── command
│   │   ├── completion
│   │   ├── container
│   │   ├── formatter
│   │   │   └── tabwriter
│   │   ├── image
│   │   │   └── build
│   │   │       └── internal
│   │   │           ├── git
│   │   │           └── urlutil
│   │   └── inspect
│   ├── config
│   │   ├── configfile
│   │   ├── credentials
│   │   ├── memorystore
│   │   └── types
│   ├── connhelper
│   │   ├── commandconn
│   │   ├── internal
│   │   │   └── syntax
│   │   └── ssh
│   ├── context
│   │   ├── docker
│   │   └── store
│   ├── debug
│   ├── flags
│   ├── hints
│   ├── streams
│   ├── trust
│   └── version
├── cli-plugins
│   ├── hooks
│   ├── manager
│   ├── metadata
│   ├── plugin
│   └── socket
├── internal
│   ├── jsonstream
│   ├── lazyregexp
│   ├── prompt
│   ├── tui
│   └── volumespec
├── opts
├── pkg
│   └── kvfile
└── templates

48 directories

@thaJeztah thaJeztah force-pushed the bump_moby_cli branch 3 times, most recently from d2146d4 to 3a7124d Compare July 24, 2025 09:24
@thaJeztah thaJeztah force-pushed the bump_moby_cli branch 3 times, most recently from 3b1ec4c to 6cd56af Compare August 5, 2025 22:51
@thaJeztah
Copy link
Member Author

Down to two packages; both possibly candidates for github.com/moby/sys;

tree -d vendor/github.com/docker/docker
vendor/github.com/docker/docker
└── pkg
    ├── pidfile
    └── process

4 directories

CLI still needs sorting out;

Details
tree -d vendor/github.com/docker/cli
vendor/github.com/docker/cli
├── cli
│   ├── command
│   │   ├── completion
│   │   ├── container
│   │   ├── formatter
│   │   │   └── tabwriter
│   │   ├── image
│   │   │   └── build
│   │   │       └── internal
│   │   │           ├── git
│   │   │           └── urlutil
│   │   ├── inspect
│   │   └── system
│   │       └── pruner
│   ├── config
│   │   ├── configfile
│   │   ├── credentials
│   │   ├── memorystore
│   │   └── types
│   ├── connhelper
│   │   ├── commandconn
│   │   ├── internal
│   │   │   └── syntax
│   │   └── ssh
│   ├── context
│   │   ├── docker
│   │   └── store
│   ├── debug
│   ├── flags
│   ├── hints
│   ├── streams
│   ├── trust
│   └── version
├── cli-plugins
│   ├── hooks
│   ├── manager
│   ├── metadata
│   ├── plugin
│   └── socket
├── internal
│   ├── jsonstream
│   ├── lazyregexp
│   ├── prompt
│   ├── registry
│   ├── tui
│   └── volumespec
├── opts
├── pkg
│   └── kvfile
└── templates

51 directories

@thaJeztah thaJeztah force-pushed the bump_moby_cli branch 2 times, most recently from d7c329f to 04ac165 Compare August 28, 2025 21:12
Comment on lines -426 to -429
// filter out useless commandConn.CloseWrite warning message that can occur
// when using a remote context that is unreachable: "commandConn.CloseWrite: commandconn: failed to wait: signal: killed"
// https://github.com/docker/cli/blob/e1f24d3c93df6752d3c27c8d61d18260f141310c/cli/connhelper/commandconn/commandconn.go#L203-L215
logrus.AddHook(logutil.NewFilter([]logrus.Level{
Copy link
Member Author

Choose a reason for hiding this comment

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

These were removed in docker/cli#6644

Comment on lines -152 to +153
text = jm.Progress.String()
// FIXME(thaJeztah): what's the replacement for Progress.String()?
// text = jm.Progress.String()
Copy link
Member Author

Choose a reason for hiding this comment

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

This one probably still needs to be looked into.

Comment on lines -408 to +425
progress = jm.Progress.String()
// FIXME(thaJeztah): what's the replacement for Progress.String()?
// progress = jm.Progress.String()
Copy link
Member Author

Choose a reason for hiding this comment

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

Same for this one

@thaJeztah thaJeztah force-pushed the bump_moby_cli branch 3 times, most recently from ce597b5 to 8797fa0 Compare January 26, 2026 08:45
@codecov
Copy link

codecov bot commented Jan 26, 2026

Comment on lines 194 to 196
func TestDefaultNetworkSettings(t *testing.T) {
t.Run("returns the network with the highest priority when service has multiple networks", func(t *testing.T) {
t.Skip("FIXME: test is failing (but for legacy API versions?)")
service := composetypes.ServiceConfig{
Copy link
Member Author

Choose a reason for hiding this comment

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

I put this as a separate commit, but this test can probably be removed, as I for legacy API versions ("1.43"), which the client no longer supports; https://github.com/moby/moby/blob/client/v0.2.2/client/client.go#L112-L114

Copy link
Member Author

Choose a reason for hiding this comment

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

I forgot what the exact failure was, so temporarily enabled it again;

=== Failed
=== FAIL: pkg/compose TestDefaultNetworkSettings/returns_the_network_with_the_highest_priority_when_service_has_multiple_networks (0.00s)
    create_test.go:225: assertion failed: expected map[myProject_myNetwork1:%!s(*network.EndpointSettings=&{0xc00017efa0 [] [myProject-myService-1 myService] map[] 0   {{0 0} {<nil>}} {{0 0} {<nil>}} [] 0 {{0 0} {<nil>}} {{0 0} {<nil>}} 0 []}) myProject_myNetwork2:%!s(*network.EndpointSettings=&{0xc00017ef50 [] [myProject-myService-1 myService] map[] 0   {{0 0} {<nil>}} {{0 0} {<nil>}} [] 0 {{0 0} {<nil>}} {{0 0} {<nil>}} 0 []})] (length 2) to have length 1
    --- FAIL: TestDefaultNetworkSettings/returns_the_network_with_the_highest_priority_when_service_has_multiple_networks (0.00s)

=== FAIL: pkg/compose TestDefaultNetworkSettings (0.00s)

So it expects a single network, but returns 2?

    create_test.go:225: assertion failed: expected
    
    map[
        myProject_myNetwork1: %!s(*network.EndpointSettings=&{0xc00017efa0 [] [myProject-myService-1 myService] map[] 0   {{0 0} {<nil>}} {{0 0} {<nil>}} [] 0 {{0 0} {<nil>}} {{0 0} {<nil>}} 0 []})
        myProject_myNetwork2: %!s(*network.EndpointSettings=&{0xc00017ef50 [] [myProject-myService-1 myService] map[] 0   {{0 0} {<nil>}} {{0 0} {<nil>}} [] 0 {{0 0} {<nil>}} {{0 0} {<nil>}} 0 []})
    ]
    
    (length 2) to have length 1

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, right, yes, indeed: that's code that was only ran on API < 1.44, but that conditional code is removed, so it will no longer return a single network (which was because older daemons didn't support that);

// Starting from API version 1.44, the Engine will take several EndpointsConfigs
// so we can pass all the extra networks we want the container to be connected to
// in the network configuration instead of connecting the container to each extra
// network individually after creation.
if versions.GreaterThanOrEqualTo(version, APIVersion144) {
if len(service.Networks) > 1 {
serviceNetworks := service.NetworksByPriority()
for _, networkKey := range serviceNetworks[1:] {
mobyNetworkName := project.Networks[networkKey].Name
epSettings := createEndpointSettings(project, service, serviceIndex, networkKey, links, useNetworkAliases)
endpointsConfig[mobyNetworkName] = epSettings
}
}
if primaryNetworkEndpoint.MacAddress == "" {
primaryNetworkEndpoint.MacAddress = service.MacAddress
}
}

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah force-pushed the bump_moby_cli branch 3 times, most recently from 1a7e8e8 to 6510727 Compare February 4, 2026 16:42
Test that the network with the highest priority is returned as
"primary" network, and other networks as extra networks.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
…ture

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
- use an intermediate serviceNetworks slice so that we don't have
  to call service.NetworksByPriority multiple times.
- shift the primary network from the slice (if any), so that
  we can drop some checks for "additional networks"
- group code related to setting up the primary network as first
  step, then append remaining networks.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants