build: improve error messages for docker driver #1998
Conversation
|
If you're willing to add integration tests like you have done for build progress that would be nice. We can do that in follow-up as well. |
dvdksn
force-pushed
the
build/docker-driver-errmsgs
branch
from
0055cd2 to
6ed6e77
Compare
|
@crazy-max I wanted to add that actually! But I wasn't sure how to make an integration test for a specific driver. Is it just checking integration.Sandbox.Name(), and skip if name != "docker"? |
Like Lines 185 to 187 in 86ae8ea |
dvdksn
force-pushed
the
build/docker-driver-errmsgs
branch
from
6ed6e77 to
7f4721b
Compare
dvdksn
force-pushed
the
build/docker-driver-errmsgs
branch
2 times, most recently
from
021bd5d to
58036c5
Compare
|
@dvdksn Pushed extra commit to split tests in dedicated funcs. We need to also skip tests for |
dvdksn
force-pushed
the
build/docker-driver-errmsgs
branch
from
01d3245 to
754cfbb
Compare
build/build.go
Outdated
| } | ||
|
|
||
| func multiPlatformBuildNotSupported(d driver.Driver) error { | ||
| return errors.Errorf(`Multi-platform build is not supported for the %s driver without the containerd image store. |
There was a problem hiding this comment.
The other cases should also work with containerd image store if we want to mention that.
dvdksn
force-pushed
the
build/docker-driver-errmsgs
branch
from
754cfbb to
2ba1043
Compare
|
Didn't realize the c8d restriction applied to all cases, so I made some refactoring |
build/build.go
Outdated
| @@ -627,7 +627,7 @@ func toSolveOpt(ctx context.Context, node builder.Node, multiDriver bool, opt Op | |||
| pp[i] = platforms.Format(p) | |||
| } | |||
| if len(pp) > 1 && !nodeDriver.Features(ctx)[driver.MultiPlatform] { | |||
| return nil, nil, notSupported(nodeDriver, driver.MultiPlatform) | |||
| return nil, nil, notSupported(driver.MultiPlatform, nodeDriver, "https://docs.docker.com/go/build-multi-platform") | |||
There was a problem hiding this comment.
Do the go-redirect URLs need a trailing slash?
There was a problem hiding this comment.
Not strictly needed; the redirect handles both cases: docker/docs@bf4a605
There was a problem hiding this comment.
Ah, yes, I was aware we were able to handle both, but I think we use URLs with a trailing / as the "canonical" one for all our links, so thought I'd be better to be consistent and use URLs ending with a /. (For the non-/go/ URLs this prevents an extra redirect).
|
@dvdksn needs rebase 🙏 |
dvdksn
force-pushed
the
build/docker-driver-errmsgs
branch
3 times, most recently
from
7c9b2e1 to
6f31958
Compare
Signed-off-by: David Karlsson <[email protected]> Co-authored-by: CrazyMax <[email protected]> Signed-off-by: David Karlsson <[email protected]>
dvdksn
force-pushed
the
build/docker-driver-errmsgs
branch
from
6f31958 to
bf5a700
Compare
|
|
||
| func testBuildCacheExportNotSupported(t *testing.T, sb integration.Sandbox) { | ||
| if sb.Name() != "docker" { | ||
| t.Skip("skipping test for non-docker workers") | ||
| } | ||
|
|
||
| dir := createTestProject(t) | ||
| cmd := buildxCmd(sb, withArgs("build", "--cache-to=type=registry", dir)) | ||
| out, err := cmd.CombinedOutput() | ||
| require.Error(t, err, string(out)) | ||
| require.Contains(t, string(out), "Cache export is not supported") | ||
| } |
There was a problem hiding this comment.
Wondering if we should start using a test-table for these tests; did a quick try what it'd look like; dvdksn#2
WDYT @crazy-max ?
There was a problem hiding this comment.
Yes sounds good but I think we should split ones about empty key from other tests
There was a problem hiding this comment.
Opened as #2041
but I think we should split ones about empty key from other tests
Any specific reason for that? (just wondering)
Signed-off-by: David Karlsson [email protected]
Improve error messages, and add documentation links, for errors related to the limited capabilities of the
dockerdriverCloses #1996