copacetic icon indicating copy to clipboard operation
copacetic copied to clipboard

feat: add push flag to support pushing to registry directly

Open RealHarshThakur opened this issue 2 years ago • 14 comments

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. https://github.com/project-copacetic/copacetic/issues/285#issuecomment-1708239927

Closes #293

RealHarshThakur avatar Sep 07 '23 18:09 RealHarshThakur

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%) :arrow_down:
pkg/patch/patch.go 20.00% <46.34%> (+13.96%) :arrow_up:
pkg/buildkit/buildkit.go 0.00% <0.00%> (ø)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Sep 07 '23 18:09 codecov[bot]

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?

sozercan avatar Sep 07 '23 23:09 sozercan

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.

jeremyrickard avatar Sep 08 '23 00:09 jeremyrickard

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 avatar Sep 08 '23 08:09 RealHarshThakur

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 avatar Sep 08 '23 09:09 RealHarshThakur

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

RealHarshThakur avatar Sep 08 '23 09:09 RealHarshThakur

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

sozercan avatar Sep 08 '23 16:09 sozercan

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

cool, I've added the registry check @sozercan

RealHarshThakur avatar Sep 11 '23 18:09 RealHarshThakur

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

RealHarshThakur avatar Sep 22 '23 14:09 RealHarshThakur

@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 :)

salaxander avatar Sep 22 '23 14:09 salaxander

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

sozercan avatar Oct 16 '23 21:10 sozercan

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

RealHarshThakur avatar Oct 17 '23 11:10 RealHarshThakur

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

RealHarshThakur avatar Nov 09 '23 19:11 RealHarshThakur

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

sozercan avatar Nov 28 '23 19:11 sozercan

Closing due to inactivity. Please feel free to re-open if there's ongoing work. Thanks!

sozercan avatar Jul 03 '24 18:07 sozercan