Uh oh!
There was an error while loading. Please reload this page.
bring back and expose BuildKitEnabled func - #3435
Conversation
Uh oh!
There was an error while loading. Please reload this page.
ndeloof
commented
Feb 23, 2022
we can manage the signature change on docker/compose, thanks for this |
| } | ||
| // otherwise, assume BuildKit is enabled but | ||
| // not if wcow reported from server side | ||
| return cli.ServerInfo().OSType != "windows", nil |
There was a problem hiding this comment.
Probably for a follow-up, but I just realise we don't take older daemons into account. (Should we default to "false" on, daemons that do not yet have BuildKit, or before BuildKit left experimental (BuildKit no longer required experimental since 18.09.0; https://docs.docker.com/engine/release-notes/18.09/#new-features)
| // wcow build command must use the legacy builder | ||
| // if not opt-in through a builder component | ||
| if !useBuilder && dockerCli.ServerInfo().OSType == "windows" { |
There was a problem hiding this comment.
I'm slightly concerned that there's now 2 implementations to do the BuildKit check (one in BuildKitEnabled()); I don't want to necessarily block this PR on that, but wondering if we can use the same code for both (so that we're sure the logic won't diverge)
There was a problem hiding this comment.
Yes, I was thinking of moving some logic to BuildKitEnabled but in processBuilder we need to know if DOCKER_BUILDKIT is being used/enforced.
There was a problem hiding this comment.
could we extract the common bits to a non-exported function perhaps? (haven't checked), so something like;
func (cli*DockerCli) BuildKitEnabled() (bool, error) {
foo, _, err:=doTheBuildKitCheck(cli)
returnfoo, err
}There was a problem hiding this comment.
hum yeah we could do that, let me see.
7fd33b9 to
b24f9c5CompareCodecov Report
@@ Coverage Diff @@## master #3435 +/- ##
==========================================
- Coverage 58.29% 58.25% -0.05%
==========================================
Files 287 287 Lines 24137 24152 +15 ==========================================
- Hits 14071 14070 -1 - Misses 9207 9222 +15 - Partials 859 860 +1 |
1d2c309 to
311772eCompareSigned-off-by: CrazyMax <crazy-max@users.noreply.github.com>
…nent Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
311772e to
e7a8748Compare
thaJeztah
left a comment
There was a problem hiding this comment.
LGTM
for those following along; @crazy-max did an implementation with the "common bits" extracted to a function, but because things are in separate packages, that meant the function had to be exported (so we couldn't put it in a non-exported / internal function).
Because of that, we decided that duplicating the code/logic for handling the DOCKER_BUILDKIT env-var (and other options) would be the better alternative for now.
Bring back
BuildKitEnabledthat is used by compose: https://github.com/docker/compose/blob/598b59f8bf0a86f3ba2a135c0489b63bb4285973/pkg/compose/build.go#L216-L219. @ndeloof Signature has changed, let me know if it sounds good to you.Second commit fixes an issue where legacy builder was not used if wcow was reported server-side (see #3314).