-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Include all networks in ContainerCreate call if API >= 1.44 #11429
Conversation
7d4030d
to
39a5531
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #11429 +/- ##
==========================================
- Coverage 58.09% 58.08% -0.02%
==========================================
Files 136 136
Lines 11543 11565 +22
==========================================
+ Hits 6706 6717 +11
- Misses 4177 4185 +8
- Partials 660 663 +3 ☔ View full report in Codecov by Sentry. |
39a5531
to
02cd50c
Compare
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.
LGTM as a general feature but I'm not a big fan we cover such an improvement with unit tests relying on API mocks. As we start relying on 1.25 features we need to run real tests on multiple engine versions
Yup! I wrote in the PR test:
Probably a good change regardless :') |
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.
LGTM! Want to rebase to pick up #11459? and then we can mergeeee
02cd50c
to
ee8d82f
Compare
Sorry, just saw this @milas! Rebased :) |
cc @glours if you want to TAL |
ee8d82f
to
b834cc3
Compare
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.
Sounds good to me 👍
Previously, we included the container's primary network in the ContainerCreate API call, and then connected the created container to each extra network one-by-one, by calling NetworkConnect. However, starting API version 1.44, the ContainerCreate endpoint now takes multiple EndpointsConfigs, allowing us to just include all the network configurations there and skip connecting the container to each extra network separately. Signed-off-by: Laura Brehm <laurabrehm@hey.com>
b834cc3
to
0fbdb93
Compare
It's failing on a ddev test:
which I think might be flaky, but I don't have permissions to restart the test to check. |
CI was stuck, I restarted the whole workflow |
Ok we forgot to change the checks since we introduced the tests against 2 Docker engine versions @ndeloof 😅 |
Previously, we included the container's primary network in the
ContainerCreate
API call, and then connected the created container to each extra network one-by-one, by callingNetworkConnect
.However, starting API version 1.44 (moby/moby#45906), the
ContainerCreate
endpoint now takes multiple networks, allowing us to just include all the network configurations there and skip connecting the container to each extra network separately.What I did
Change
composeService.createMobyContainer
to include all the network configurations for the container in theContainerCreate
call and skip connecting the container to networks individually, when API version >= 1.44.Also added some tests to make sure we don't break this for <1.44.
(a good callout here is that currently, afaik, Compose isn't being tested against different Engine versions in CI, so it may be worth it to do that soon/wait on that before merging this PR)
(not mandatory) A picture of a cute animal, if possible in relation to what you did