feat: add push flag to support pushing to registry directly #294

RealHarshThakur · 2023-09-07T18:06:47Z

This PR enables direct push to registry from buildkit:

copa patch -i docker.io/library/alpine:3.16.4 -r patch.json -p ttl.sh/alpinepatched:1h

One side-effect of this would be that copa wouldn't rely on Docker anymore if there is a remote buildkit setup as I mentioned here. #285 (comment)

Closes #293

codecov · 2023-09-07T18:09:30Z

Codecov Report

Attention: 63 lines in your changes are missing coverage. Please review.

Comparison is base (2b9f177) 33.02% compared to head (82060be) 33.49%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #294      +/-   ##
==========================================
+ Coverage   33.02%   33.49%   +0.46%     
==========================================
  Files          17       17              
  Lines        1626     1663      +37     
==========================================
+ Hits          537      557      +20     
- Misses       1060     1072      +12     
- Partials       29       34       +5

Files	Coverage Δ
pkg/patch/cmd.go	`47.91% <25.00%> (-0.98%)`	⬇️
pkg/patch/patch.go	`20.00% <46.34%> (+13.96%)`	⬆️
pkg/buildkit/buildkit.go	`0.00% <0.00%> (ø)`

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

sozercan · 2023-09-07T23:42:43Z

I am wondering if we should align with buildx CLI here. Ideally, we don't want to break existing users so what I was thinking was to use --tag to accept registy/repo:tag and --push can be a bool. to remain backwards compat, we can parse if --tag includes : and /. If it includes both : and /, retag the entire image. If it doesn't include both : and /, only replace the tag.

-i foo.io/repo/image:tag -t -patched -> foo.io/repo/image:tag-patched (what we have today)
-i foo.io/repo/image:tag -t bar.io/repo/image:tag -> bar.io/repo/image:tag
-i foo.io/repo/image:tag -t bar.io/repo/image -> invalid (must provide tag)
-i foo.io/repo/image:tag -t repo/image:tag -> invalid (must provide registry)

@jeremyrickard @cpuguy83 wdyt?

jeremyrickard · 2023-09-08T00:12:54Z

I am wondering if we should align with buildx CLI here. Ideally, we don't want to break existing users so what I was thinking was to use --tag to accept registy/repo:tag and --push can be a bool. to remain backwards compat, we can parse if --tag includes : and /. If it includes both : and /, retag the entire image. If it doesn't include both : and /, only replace the tag.

-i foo.io/repo/image:tag -t -patched -> foo.io/repo/image:tag-patched (what we have today) -i foo.io/repo/image:tag -t bar.io/repo/image:tag -> bar.io/repo/image:tag -i foo.io/repo/image:tag -t bar.io/repo/image -> invalid (must provide tag) -i foo.io/repo/image:tag -t repo/image:tag -> invalid (must provide registry)

@jeremyrickard @cpuguy83 wdyt?

I like the -i foo.io/repo/image:tag -t bar.io/repo/image:tag syntax because it's also very explicit.

I also think that adding a --push flag would be useful, especially if it didn't require docker locally. Not the same scenario, but the experience withko and pushing images w/o docker is super useful.

RealHarshThakur · 2023-09-08T08:45:03Z

cool, happy to go with what you folks suggested.

Although, I am not sure about this bit:

-i foo.io/repo/image:tag -t repo/image:tag -> invalid (must provide registry)

This might still be what the user wants, partially due to how popular dockerhub is: username/image:tag is valid over there. Also, other such registries like ttl.sh also don't mind that format.

Even this bit, might be specific to how registries should manage it. Many registries would just accept it and tag it as latest(although, not encouraged):

 -i foo.io/repo/image:tag -t bar.io/repo/image -> invalid (must provide tag)

RealHarshThakur · 2023-09-08T09:42:10Z

I've pushed a commit to get most of what you folks suggested. Added couple test cases that shows we'll be backwards-compatible and a new case for specifying a different registry. I've tried to stay unopinionated about destination registry conventions as copa will throw an error during push if buildkit fails to push anyway. Happy to take your suggestions tho

RealHarshThakur · 2023-09-08T09:48:20Z

I think setting up the mock buildkit API in unit tests failed, is it acting flaky?

sozercan · 2023-09-08T16:51:59Z

This might still be what the user wants, partially due to how popular dockerhub is: username/image:tag is valid over there. Also, other such registries like ttl.sh also don't mind that format.

this is being consistent with -i since you must provide docker.io there

RealHarshThakur · 2023-09-11T18:17:18Z

this is being consistent with -i since you must provide docker.io there

cool, I've added the registry check @sozercan

RealHarshThakur · 2023-09-22T14:40:43Z

@sozercan @jeremyrickard don't mean to badger, is there anything else you'd like me to change on this PR?

salaxander · 2023-09-22T14:59:30Z

@RealHarshThakur sorry for the delay - @sozercan has been on vacation for the past couple days. I'll see if I can get you a review asap :)

sozercan

Sorry about the delay! Please see comments

sozercan · 2023-10-03T17:49:12Z

pkg/buildkit/buildkit.go

@@ -232,3 +232,47 @@ func SolveToDocker(ctx context.Context, c *client.Client, st *llb.State, configD
 	})
 	return eg.Wait()
 }
+
+func SolveToRegistry(ctx context.Context, c *client.Client, st *llb.State, configData []byte, tag string) error {


this seems like duplicating the existing code, can we refactor to share the logic?

sozercan · 2023-10-03T17:57:51Z

pkg/patch/patch.go

+		return nil, err
+	}
+	if ref.IsNameOnly(imageName) {
+		log.Warnf("Image name has no tag or digest, using latest as tag")


Suggested change

log.Warnf("Image name has no tag or digest, using latest as tag")

log.Warn("image name has no tag or digest, using latest as tag")

although not sure if we get to this point, since i believe it'll fast fail before this

sozercan · 2023-10-03T20:51:18Z

pkg/patch/patch_test.go

+		{
+			name:       "tag passed but without registry or repo",
+			image:      "docker.io/library/nginx:1.21.3",
+			patchedTag: "my.registry/nginx:1.21.3-patched",


this should be valid (something like myregistry.azurecr.io/nginx:1.21.3-patched)

sozercan · 2023-10-03T20:53:07Z

pkg/patch/patch.go

+	if slashCount > 0 {
+		if slashCount < 2 {
+			err := fmt.Errorf("invalid tag %s, must be in the form <registry>/<image>:<tag>", patchedTag)
+			log.Error(err)


i think this is duplicate, since return is logged already?

sozercan · 2023-10-16T21:39:32Z

ping @RealHarshThakur. we are getting close to cutting v0.5.0 release if you want this to be included

RealHarshThakur · 2023-10-17T11:55:40Z

I'll try but most likely, I'll be able to pick it up by end of the month

Signed-off-by: Harsh Thakur <harshthakur9030@gmail.com>

RealHarshThakur · 2023-11-09T19:54:46Z

@sozercan sorry for the delay, I've made the changes you've requested.

sozercan · 2023-11-28T19:54:22Z

@RealHarshThakur thanks! can you please rebase when you get a chance? looks there are a few conflicts now.

RealHarshThakur requested review from jeremyrickard, salaxander and sozercan as code owners September 7, 2023 18:06

RealHarshThakur changed the title ~~Add push flag to support pushing to registry directly~~ feat: add push flag to support pushing to registry directly Sep 7, 2023

RealHarshThakur force-pushed the pushimages branch from c3f71ed to 86b794b Compare September 8, 2023 09:33

RealHarshThakur force-pushed the pushimages branch 2 times, most recently from 66fcf39 to 46f56f1 Compare September 9, 2023 08:26

RealHarshThakur force-pushed the pushimages branch 2 times, most recently from cb525c9 to 7e736d6 Compare September 22, 2023 14:38

sozercan modified the milestone: v0.5.0 Oct 3, 2023

sozercan requested changes Oct 3, 2023

View reviewed changes

RealHarshThakur added 4 commits November 10, 2023 00:38

Add push flag to support pushing to registry directly

e0a7b09

Signed-off-by: Harsh Thakur <harshthakur9030@gmail.com>

add validation for registry/repo/image being passed

44ac19e

Signed-off-by: Harsh Thakur <harshthakur9030@gmail.com>

fix logs and image registry names

8fb40f0

Signed-off-by: Harsh Thakur <harshthakur9030@gmail.com>

Dedup Solve methods

82060be

Signed-off-by: Harsh Thakur <harshthakur9030@gmail.com>

RealHarshThakur force-pushed the pushimages branch from d689d89 to 82060be Compare November 9, 2023 19:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add push flag to support pushing to registry directly #294

feat: add push flag to support pushing to registry directly #294

RealHarshThakur commented Sep 7, 2023 •

edited

codecov bot commented Sep 7, 2023 •

edited

sozercan commented Sep 7, 2023

jeremyrickard commented Sep 8, 2023 •

edited

RealHarshThakur commented Sep 8, 2023 •

edited

RealHarshThakur commented Sep 8, 2023

RealHarshThakur commented Sep 8, 2023

sozercan commented Sep 8, 2023

RealHarshThakur commented Sep 11, 2023

RealHarshThakur commented Sep 22, 2023

salaxander commented Sep 22, 2023

sozercan left a comment

sozercan Oct 3, 2023

sozercan Oct 3, 2023

sozercan Oct 3, 2023

sozercan Oct 3, 2023

sozercan Oct 3, 2023

sozercan commented Oct 16, 2023

RealHarshThakur commented Oct 17, 2023

RealHarshThakur commented Nov 9, 2023

sozercan commented Nov 28, 2023

	log.Warnf("Image name has no tag or digest, using latest as tag")
	log.Warn("image name has no tag or digest, using latest as tag")

feat: add push flag to support pushing to registry directly #294

Are you sure you want to change the base?

feat: add push flag to support pushing to registry directly #294

Conversation

RealHarshThakur commented Sep 7, 2023 • edited

codecov bot commented Sep 7, 2023 • edited

Codecov Report

sozercan commented Sep 7, 2023

jeremyrickard commented Sep 8, 2023 • edited

RealHarshThakur commented Sep 8, 2023 • edited

RealHarshThakur commented Sep 8, 2023

RealHarshThakur commented Sep 8, 2023

sozercan commented Sep 8, 2023

RealHarshThakur commented Sep 11, 2023

RealHarshThakur commented Sep 22, 2023

salaxander commented Sep 22, 2023

sozercan left a comment

Choose a reason for hiding this comment

sozercan Oct 3, 2023

Choose a reason for hiding this comment

sozercan Oct 3, 2023

Choose a reason for hiding this comment

sozercan Oct 3, 2023

Choose a reason for hiding this comment

sozercan Oct 3, 2023

Choose a reason for hiding this comment

sozercan Oct 3, 2023

Choose a reason for hiding this comment

sozercan commented Oct 16, 2023

RealHarshThakur commented Oct 17, 2023

RealHarshThakur commented Nov 9, 2023

sozercan commented Nov 28, 2023

RealHarshThakur commented Sep 7, 2023 •

edited

codecov bot commented Sep 7, 2023 •

edited

jeremyrickard commented Sep 8, 2023 •

edited

RealHarshThakur commented Sep 8, 2023 •

edited