-
Notifications
You must be signed in to change notification settings - Fork 53
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
add support for socketless image copy #63
Conversation
Thanks for your work on this @hassenius ❤️ , I think all of this makes sense. Running without the Daemon has been something desired for quite some time. |
# SINKER_VERSION is set during the release process | ||
ARG SINKER_VERSION=0.0.0 | ||
RUN go build -ldflags="-X 'github.com/plexsystems/sinker/internal/commands.sinkerVersion=${SINKER_VERSION}'" | ||
RUN go build -tags 'containers_image_openpgp' -ldflags="-X 'github.com/plexsystems/sinker/internal/commands.sinkerVersion=${SINKER_VERSION}'" |
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.
@jpreese fyi, this will affect the build pipeline too.
As per https://github.com/containers/image/blob/main/README.md#supported-build-tags
Use a Golang-only OpenPGP implementation for signature verification instead of the default cgo/gpgme-based implementation; the primary downside is that creating new signatures with the Golang-only implementation is not supported.
Which should not affect us, as I don't think generating signatures would be outside the problem domain sinker addresses.
@jpreese how do you feel about the separate |
How would copy differ from sync? |
You mean to name the command |
Yeah, instead of introducing a new command, just have the current sync command use your work. It just wasn't clear to me if you're also proposing behavioral differences. |
Not sure which sync command you refer to @jpreese ?
I think the new command is quite fundamentally different from from the pull/push implementations, since it doesn't use the image store managed by the docker daemon, so I think it makes sense to keep it separate from the current push/pull commands. |
I agree that the mechanics differ quite a bit between the current commands and the only you're proposing, but the semantics feel the same. That is, whatever is defined in You're deeper into the woods than I am on this, so if you feel they are different enough to warrant more commands and it fits your flow better--I am for it. Just wanted to make sure we've considered everything. |
So, this is leveraging what's implemented in https://github.com/containers/image/blob/main/copy/copy.go, and the code is mainly just setting up what's needed for that. I think the main difference being that the focus on the implementation in copy.go is to ensure an identical copy, including manifest digests, in pull/push it's not. That's a subtle difference that can be a breaking change for some folks. I wouldn't be too keen on pulling apart the implementation in copy.go to fit it into existing pull/push commands, but I guess it would be possible to replace the existing pull/push with just this implementation as push (probably removing pull since it doesn't make much sense anymore). I think from the intended usage pattern of sinker this would be fine, but it could be breaking if anyone does anything outside that, like for example pull, modify, push (i.e. add labels/annotations, whatever) -- which I think technically would be supported with the current implementation? |
Anything further needed from me @jpreese ? |
Shouldn't be, @hassenius! I'm at KubeCon EU at the moment so activity is spotty. But I will take a look at the PR as soon as I can ❤️ |
Me too, we should meet for a coffee |
1 similar comment
Me too, we should meet for a coffee |
Spoke at KubeCon -- this is already being used in production, so has been battle tested. Code itself looks OK to me. Will do a couple cleanups in a followup PR. |
This proposal adds support for github.com/containers/image/v5 (addesses #47) which enables copying of images from source to target without requiring access to docker socket.
This is necessary for us to enable use of sinker in a containerised pipeline without docker-in-docker enabled.
I also started an outline of how to address #62 via boolean
--all-variants
and--override-os
/--override-arch
It currently works (without docker running on a mac)
I would like as a next step add the option to direct this, as a cleaner part of command line and image manifest.
For example could be something like
Or something like that..
Please let me know your thoughts, and I'll should be able to wrap it up quite quickly...