containerimage: Export option keys by vvoland · Pull Request #3694 · moby/buildkit

vvoland · 2023-03-08T15:23:53Z

Make containerdimage attribute key names public, to make it easier for some downstream projects to use them without hardcoding them on their side 😅 .

thaJeztah

LGTM

thaJeztah · 2023-03-08T15:44:37Z

exporter/containerimage/export.go

+	KeyPush           = "push"
+	KeyPushByDigest   = "push-by-digest"
+	KeyInsecure       = "registry.insecure"
+	KeyUnpack         = "unpack"
+	KeyDanglingPrefix = "dangling-name-prefix"
+	KeyNameCanonical  = "name-canonical"
+	KeyStore          = "store"


As a follow-up, we should consider;

making these strong-typed (if possible)

documenting these (some of them may be clear, but still may require some knowledge (what does this mean?)

I know @jedevc has done some work on improving such consts in other areas (happy to do the work, but I may need some input on the docs side of things)

tonistiigi

We could do this in a follow-up but these constants should not be defined in this pkg if they are public. If you want to use these keys, eg. to pass them from the client side, you should not need to pull in the server side implementation with all its dependencies.

This could either be in exptypes, or in a separate subpackage.

thaJeztah · 2023-03-08T15:54:29Z

these contacts should not be defined in this pkg if they are public

Good point, I was wondering (should've asked) if this was the right place for these; are these consts "common" between exporters, or specific to each?

FWIW; this change is not "urgent" (we currently just duplicate them in moby/moby) but would be good to have in the next (v0.12) release, so I'd be fine to give it a bit more look and "do it correct" in a single PR (no need to rush this in). Perhaps that also allows us to combine it with #3694 (comment)

thaJeztah

changing my "LGTM" to "request changes".

As there's no rush, I think it's worth spending a bit more time on this to "do it right"

vvoland · 2023-03-08T15:55:30Z

Yeah, good point. I'll rework this PR.

jedevc · 2023-05-23T11:32:01Z

@vvoland @thaJeztah, I imagine there's a good chance you might want these in the next buildkit release?

thaJeztah · 2023-05-23T11:33:57Z

Looks like good to have (not critical but definitely good to have).

Do we have pointers what package those consts should move to?

vvoland · 2023-05-23T11:35:26Z

exporter/containerimage/exptypes sounds reasonable. I'll try to do it today.

vvoland · 2023-05-23T15:38:24Z

Done, also added some docs, please let me know if I wrote something silly there 😄

thaJeztah · 2023-05-23T15:56:28Z

@jedevc any thoughts on #3694 (comment) (in case we want a type for these?)

jedevc · 2023-05-24T08:53:52Z

I think having a custom separate type (that's not just an alias) will cause a lot of churn in different places. The exporter type is passed as a plain string through the BuildKit control API, so we'd have to convert it out of that constantly.

Nothing else in exptypes has its custom type, so I'm happy to leave it as-is for - we can also revisit later 🎉

exporter/containerimage/exptypes/keys.go

thaJeztah · 2023-05-24T09:45:36Z

The exporter type is passed as a plain string through the BuildKit control API, so we'd have to convert it out of that constantly.

Yes, makes sense. I had #3199 in mind, where we ultimately decided to go for strong-typed (which wasn't too bad w.r.t. casting strings to types), but must admit I haven't looked how much impact the consts in this PR would have. (Was just considering: if we want to do it, this might be a good occasion to do so)

vvoland · 2023-05-24T12:08:55Z

Moved other exporter keys and changed the keys to be strongly typed. It doesn't seem to involve that much type-casting, although it makes it a bit weird for the external consumer to assign them to a map[string]string anyway. I can drop the last commit if we don't want it.

control/control.go

exporter/util/epoch/parse.go

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland · 2023-05-30T11:40:15Z

Failure looks unrelated:

=== FAIL: client TestIntegration/TestBasicGhaCacheImportExport/worker=containerd (302.33s)
    client_test.go:4804: 
        	Error Trace:	/src/client/client_test.go:4804
        	            				/src/client/client_test.go:5190
        	            				/src/util/testutil/integration/run.go:89
        	            				/src/util/testutil/integration/run.go:198
        	Error:      	Received unexpected error:
        	            	maximum timeout reached
        	            	github.com/moby/buildkit/util/stack.Enable
        	            		/src/util/stack/stack.go:77
        	            	github.com/moby/buildkit/util/grpcerrors.FromGRPC
        	            		/src/util/grpcerrors/grpcerrors.go:198
        	            	github.com/moby/buildkit/util/grpcerrors.UnaryClientInterceptor
        	            		/src/util/grpcerrors/intercept.go:41
        	            	google.golang.org/grpc.(*ClientConn).Invoke
        	            		/src/vendor/google.golang.org/grpc/call.go:35
        	            	github.com/moby/buildkit/api/services/control.(*controlClient).Solve
        	            		/src/api/services/control/control.pb.go:2208
        	            	github.com/moby/buildkit/client.(*Client).solve.func2
        	            		/src/client/solve.go:258
        	            	golang.org/x/sync/errgroup.(*Group).Go.func1
        	            		/src/vendor/golang.org/x/sync/errgroup/errgroup.go:75
        	            	runtime.goexit
        	            		/usr/local/go/src/runtime/asm_amd64.s:1598
        	            	failed to solve
        	            	github.com/moby/buildkit/client.(*Client).solve.func2
        	            		/src/client/solve.go:273
        	            	golang.org/x/sync/errgroup.(*Group).Go.func1
        	            		/src/vendor/golang.org/x/sync/errgroup/errgroup.go:75
        	            	runtime.goexit
        	            		/usr/local/go/src/runtime/asm_amd64.s:1598
        	Test:       	TestIntegration/TestBasicGhaCacheImportExport/worker=containerd

thaJeztah · 2023-05-31T22:49:47Z

Failure looks unrelated:

I gave CI a kick

thaJeztah · 2023-05-31T22:55:53Z

Not for this PR, but while looking at a ticket in moby/moby (moby/moby#45665), I discovered more strings in there that have non-exported consts defined in BuildKit;
https://github.com/moby/moby/blob/8d67d0c1a8d28ad4014444edc6b9975f9877b1d6/builder/builder-next/builder.go#L299-L332

consts are in the dockerfile frontend;

buildkit/frontend/dockerui/config.go

Lines 26 to 51 in 80b91c5

    
           const ( 
        
           	buildArgPrefix = "build-arg:" 
        
           	labelPrefix    = "label:" 
        
           	keyTarget           = "target" 
        
           	keyCgroupParent     = "cgroup-parent" 
        
           	keyForceNetwork     = "force-network-mode" 
        
           	keyGlobalAddHosts   = "add-hosts" 
        
           	keyHostname         = "hostname" 
        
           	keyImageResolveMode = "image-resolve-mode" 
        
           	keyMultiPlatform    = "multi-platform" 
        
           	keyNoCache          = "no-cache" 
        
           	keyShmSize          = "shm-size" 
        
           	keyTargetPlatform   = "platform" 
        
           	keyUlimit           = "ulimit" 
        
           	keyCacheFrom        = "cache-from"    // for registry only. deprecated in favor of keyCacheImports 
        
           	keyCacheImports     = "cache-imports" // JSON representation of []CacheOptionsEntry 
        
           	// Don't forget to update frontend documentation if you add 
        
           	// a new build-arg: frontend/dockerfile/docs/reference.md 
        
           	keyCacheNSArg           = "build-arg:BUILDKIT_CACHE_MOUNT_NS" 
        
           	keyMultiPlatformArg     = "build-arg:BUILDKIT_MULTI_PLATFORM" 
        
           	keyHostnameArg          = "build-arg:BUILDKIT_SANDBOX_HOSTNAME" 
        
           	keyContextKeepGitDirArg = "build-arg:BUILDKIT_CONTEXT_KEEP_GIT_DIR" 
        
           	keySourceDateEpoch      = "build-arg:SOURCE_DATE_EPOCH" 
        
           )

Perhaps we should look at those as well 🤔 - let me create a ticket for that.

thaJeztah · 2023-05-31T23:00:19Z

^^ opened frontend: export frontend options / consts for external consumers #3924

thaJeztah

LGTM! ❤️

for those interested; docs look like this;

exporter/containerimage/annotations.go

exporter/util/epoch/parse.go

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland mentioned this pull request Mar 8, 2023

c8d/builder: Store untagged images as dangling moby/moby#45123

Merged

AkihiroSuda approved these changes Mar 8, 2023

View reviewed changes

thaJeztah approved these changes Mar 8, 2023

View reviewed changes

tonistiigi reviewed Mar 8, 2023

View reviewed changes

thaJeztah requested changes Mar 8, 2023

View reviewed changes

vvoland force-pushed the containerimage-consts-public branch 2 times, most recently from 43b79f7 to c3fdce5 Compare May 23, 2023 15:22

jedevc reviewed May 24, 2023

View reviewed changes

exporter/containerimage/exptypes/keys.go Show resolved Hide resolved

vvoland force-pushed the containerimage-consts-public branch from c3fdce5 to 718b1a8 Compare May 24, 2023 12:08

vvoland changed the title ~~containerimage: Make attribute keys consts public~~ containerimage: Export option keys May 24, 2023

jedevc reviewed May 30, 2023

View reviewed changes

control/control.go Outdated Show resolved Hide resolved

thaJeztah reviewed May 30, 2023

View reviewed changes

exporter/util/epoch/parse.go Show resolved Hide resolved

containerimage: Export option keys

4fc2d7b

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland force-pushed the containerimage-consts-public branch 2 times, most recently from 6d6b719 to 6443a6a Compare May 30, 2023 11:01

jedevc approved these changes May 30, 2023

View reviewed changes

jedevc requested review from thaJeztah and tonistiigi May 30, 2023 11:12

thaJeztah mentioned this pull request May 31, 2023

frontend: export frontend options / consts for external consumers #3924

Open

thaJeztah approved these changes May 31, 2023

View reviewed changes

tonistiigi reviewed Jun 1, 2023

View reviewed changes

exporter/containerimage/annotations.go Outdated Show resolved Hide resolved

exporter/util/epoch/parse.go Show resolved Hide resolved

vvoland added 2 commits June 2, 2023 09:38

exporter/image/exptypes: Make strongly typed

cecab8d

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

exporter: Add exptypes with Common exporter keys

d82a803

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>

vvoland force-pushed the containerimage-consts-public branch from b1d3836 to d82a803 Compare June 2, 2023 07:39

tonistiigi approved these changes Jun 2, 2023

View reviewed changes

tonistiigi merged commit ab97675 into moby:master Jun 2, 2023

thaJeztah mentioned this pull request Jun 8, 2023

Support for building overlaybd images #3867

Merged

crazy-max mentioned this pull request Nov 7, 2024

Flaky test: TestBasicGhaCacheImportExport #5494

Closed

Conversation

vvoland commented Mar 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Mar 8, 2023

Choose a reason for hiding this comment

Uh oh!

tonistiigi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Mar 8, 2023

Uh oh!

thaJeztah left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vvoland commented Mar 8, 2023

Uh oh!

jedevc commented May 23, 2023

Uh oh!

thaJeztah commented May 23, 2023

Uh oh!

vvoland commented May 23, 2023

Uh oh!

vvoland commented May 23, 2023

Uh oh!

thaJeztah commented May 23, 2023

Uh oh!

jedevc commented May 24, 2023

Uh oh!

Uh oh!

thaJeztah commented May 24, 2023

Uh oh!

vvoland commented May 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vvoland commented May 30, 2023

Uh oh!

thaJeztah commented May 31, 2023

Uh oh!

thaJeztah commented May 31, 2023

Uh oh!

thaJeztah commented May 31, 2023

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

vvoland commented Mar 8, 2023 •

edited

Loading

tonistiigi left a comment •

edited

Loading

thaJeztah left a comment •

edited

Loading

vvoland commented May 24, 2023 •

edited

Loading