-
Notifications
You must be signed in to change notification settings - Fork 664
build: add OCI artifact default opt-out #3991
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -79,6 +79,7 @@ var bakeTests = []func(t *testing.T, sb integration.Sandbox){ | |||||||||||||||||
| testBakeMetadataWarningsDedup, | ||||||||||||||||||
| testBakeMultiExporters, | ||||||||||||||||||
| testBakeLoadPush, | ||||||||||||||||||
| testBakeNoDefaultOCIArtifact, | ||||||||||||||||||
| testBakeListTargets, | ||||||||||||||||||
| testBakeListVariables, | ||||||||||||||||||
| testBakeListTypedVariables, | ||||||||||||||||||
|
|
@@ -2232,6 +2233,41 @@ target "default" { | |||||||||||||||||
| // TODO: test metadata file when supported by multi exporters https://github.com/docker/buildx/issues/2181 | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| func testBakeNoDefaultOCIArtifact(t *testing.T, sb integration.Sandbox) { | ||||||||||||||||||
| if isMobyWorker(sb) { | ||||||||||||||||||
| t.Skip("attestations are not supported by the docker worker") | ||||||||||||||||||
| } | ||||||||||||||||||
|
Comment on lines
+2237
to
+2239
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Interesting; are these two the same? (I went looking if we had a more specific "supports attestations" instead of driver-name 😂) Lines 126 to 129 in 1988826
Lines 136 to 139 in 1988826
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hum indeed, should rework this 😅 |
||||||||||||||||||
|
|
||||||||||||||||||
| registry, err := sb.NewRegistry() | ||||||||||||||||||
| if errors.Is(err, integration.ErrRequirements) { | ||||||||||||||||||
| t.Skip(err.Error()) | ||||||||||||||||||
| } | ||||||||||||||||||
| require.NoError(t, err) | ||||||||||||||||||
| target := registry + "/buildx/bake-no-default-oci-artifact:latest" | ||||||||||||||||||
|
|
||||||||||||||||||
| dockerfile := []byte(` | ||||||||||||||||||
| FROM scratch | ||||||||||||||||||
| COPY foo /foo | ||||||||||||||||||
| `) | ||||||||||||||||||
| bakefile := fmt.Appendf(nil, ` | ||||||||||||||||||
| target "default" { | ||||||||||||||||||
| output = ["type=image,name=%s,push=true"] | ||||||||||||||||||
| attest = ["type=provenance"] | ||||||||||||||||||
| } | ||||||||||||||||||
| `, target) | ||||||||||||||||||
| dir := tmpdir( | ||||||||||||||||||
| t, | ||||||||||||||||||
| fstest.CreateFile("docker-bake.hcl", bakefile, 0600), | ||||||||||||||||||
| fstest.CreateFile("Dockerfile", dockerfile, 0600), | ||||||||||||||||||
| fstest.CreateFile("foo", []byte("foo"), 0600), | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
| out, err := bakeCmd(sb, withDir(dir), withEnv("BUILDX_NO_DEFAULT_OCI_ARTIFACT=true")) | ||||||||||||||||||
| require.NoError(t, err, string(out)) | ||||||||||||||||||
|
|
||||||||||||||||||
| requireLegacyAttestationStorage(t, sb, target) | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
||||||||||||||||||
| func testBakeLoadPush(t *testing.T, sb integration.Sandbox) { | ||||||||||||||||||
| if !isDockerContainerWorker(sb) { | ||||||||||||||||||
| t.Skip("only testing with docker-container worker") | ||||||||||||||||||
|
|
||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
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.
Thinking out loud; what are the formats we (currently) support for including attestations? Would it make sense to have a "attestations-format" selection instead of a boolean? (e.g.
BUILDX_ATTESTATIONS_FORMAT=(oci|foo|bar|compat|legacy)) or was the old format not complying to any standards? (ISTR there were options for legacy registries, so those would still be valid and potentially work around the issue with non-OCI-compliant registries?)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'd rather keep this as a narrow opt-out for now (specially since this change aims for a patch release). BuildKit currently exposes this as a boolean exporter option,
oci-artifact=true|false, so mirroring that asBUILDX_NO_DEFAULT_OCI_ARTIFACTkeeps Buildx from inventing a new format-selection layer on top.The explicit format selection already exists through
--output type=image,oci-artifact=falsewhen users want to choose it directly. This env var only changes the default when the option was not set, similar toBUILDX_NO_DEFAULT_ATTESTATIONS.A generic
BUILDX_ATTESTATIONS_FORMAT=legacy|compat|ocifeels a bit too broad to me because Buildx would then need to define what those names mean and keep mapping them to BuildKit exporter behavior. If BuildKit grows more attestation storage formats later, I think that should first be exposed as a BuildKit exporter option, and Buildx can follow that model.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.
Yeah, that's ok with me; mostly wanted to avoid having to introduce yet-another env-var if there's more options than enable/disable (not even sure what format the "disable" means and if that's a legacy fallback defined by OCI?)