-
Notifications
You must be signed in to change notification settings - Fork 5.7k
migrate to moby modules #13078
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
migrate to moby modules #13078
Conversation
a5b199b to
73bb946
Compare
|
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 | |||
There was a problem hiding this comment.
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 😞
|
Remaining uses of docker/docker; |
|
Build failure looks like it may be an incompatible change in buildx itself? |
|
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 |
83ca48f to
13827c4
Compare
|
|
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) |
|
Also included a (WIP) branch for docker/cli#6202 Before/After; DetailsBefore: After: |
d2146d4 to
3a7124d
Compare
3b1ec4c to
6cd56af
Compare
|
Down to two packages; both possibly candidates for CLI still needs sorting out; Details |
6cd56af to
8925b9c
Compare
d7c329f to
04ac165
Compare
0cfb12d to
53b4d98
Compare
083a7c9 to
cb07967
Compare
| // 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{ |
There was a problem hiding this comment.
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
| text = jm.Progress.String() | ||
| // FIXME(thaJeztah): what's the replacement for Progress.String()? | ||
| // text = jm.Progress.String() |
There was a problem hiding this comment.
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.
| progress = jm.Progress.String() | ||
| // FIXME(thaJeztah): what's the replacement for Progress.String()? | ||
| // progress = jm.Progress.String() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same for this one
ce597b5 to
8797fa0
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
8797fa0 to
b6784a8
Compare
b6784a8 to
3dbbaa6
Compare
| 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{ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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);
Lines 523 to 539 in af0029a
| // 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 | |
| } | |
| } |
3dbbaa6 to
8bf3cdc
Compare
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>
1a7e8e8 to
6510727
Compare
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>
6510727 to
fae6d96
Compare
What I did
Related issue
(not mandatory) A picture of a cute animal, if possible in relation to what you did