-
Notifications
You must be signed in to change notification settings - Fork 779
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
DO NOT MERGE: OCI ImageDestination #102
Conversation
dcea9d0
to
bb74d94
Compare
@@ -10,19 +10,31 @@ import ( | |||
|
|||
// FIXME: Should we just use docker/distribution and docker/docker implementations directly? | |||
|
|||
// FIXME(runcom, mitr): should we havea mediatype pkg?? |
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 don’t quite feel we need to have a separate package for everything. Sure, use packages to simplify dependencies and avoid loops and have clear separation between transports, conversions and signatures, and from time to time to simplify the callers’ notation to avoid too many words in a symbol reference, but at some point adding a complex difficult-to-navigate package structure could make understanding more difficult instead of simpler.
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.
👍
|
|
||
func (d *ociImageDestination) PutSignatures(signatures [][]byte) error { | ||
// TODO | ||
return nil |
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.
If this TODO comment is not a short-term thing, the method should return an error instead of silently throwing away data.
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.
👍
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.
but wait, then skopeo copy can't work well because it always error out on failed to upload signatures. any idea?
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.
if len(signatures) != 0 { return $someError }
.
At the moment images with signatures are nonexistent, so the above would work well enough. Later we will need a skopeo copy --drop-signatures
.
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.
alright, that makes sense, wasn't sure about that
would be cool - pls do if you can
right the issue here is that we have so many vars called |
#103 . |
func NewOCIImageDestination(dir string) (types.ImageDestination, error) { | ||
// oci dest follow docker's one - parse those here | ||
// there's no clear direction on naming though but since image-spec is WIP just assume this for now.. | ||
ref, tag, err := parseDockerImageName(dir) |
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.
@mtrmac this is important but I guess it's fine as it is right now :(
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.
In general paths are not valid Docker Image Names; Is even /home/mitr/some-dir
valid? /home/mitr/I_LIKE_CAPS/some_dir
definitely isn’t valid.
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.
right, that's why we error out if the destination name in oci:docker.io/fedora
isn't a valid reference.
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.
AFAICS d.ref
is only used for FullName
(i.e. to turn it back into a string), this seems not worth all the effort; just find the last :
and split into directory + tag?
(Have I ever mentioned that I hate mini-languages and strings and in-band signalling in general, a tiny bit? :) Yeah yeah, we can’t avoid strings for command line.)
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.
(Have I ever mentioned that I hate mini-languages and strings and in-band signalling in general, a tiny bit? :) Yeah yeah, we can’t avoid strings for command line.)
haha now I know it
AFAICS d.ref is only used for FullName (i.e. to turn it back into a string), this seems not worth all the effort; just find the last : and split into directory + tag?
was smelling to me as well - I'll do it
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.
just find the last : and split into directory + tag?
how do we make sure directory is valid? valid as in docker.io/fedora
for instance, I think we should require oci:
prefix to enforce a valid destination, or not?
Copying to preserve:
|
ca327a8
to
947cc0e
Compare
@mtrmac Does it look relatively good to go and try to merge with #103? Otherwise, I though about merging this as is and I'll follow on carrying #103 and adapting it for OCI - just so we have some OCIness into skopeo master, does this sound resonable? |
ab5cba6
to
c3be1ba
Compare
if err := d.ensureParentDirectoryExists("refs"); err != nil { | ||
return err | ||
} | ||
// TODO(runcom): |
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.
this is the only big todo left
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.
@mtrmac ptal at the last commit
func (d *ociImageDestination) ensureParentDirectoryExists(parent string) error { | ||
path := filepath.Join(d.dir, parent) | ||
if _, err := os.Stat(path); err != nil && os.IsNotExist(err) { | ||
if err := os.MkdirAll(path, 0700); err != nil { |
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.
Use 0755 perhaps? This is using 0700, and files within these directories are created using 0644. They don’t have to be consistent (the 0700 on the directory prevails) but it would be nice for them to be.
Read this line by line, that should be all. |
On it, thanks a lot |
@mtrmac test failures seems related? |
6328231
to
49c2d26
Compare
Yeah; without actually placing any breakpoints in, it seems that this changes the manifest we get from the Docker Hub from v1s1 to v2s2, and the OpenShift registry does not support v2s2 and rejects it. Oops. A short-term fix would be to either find s1-only images hosted on the Docker Hub (do they exist, or have they converted everything to s2?), or perhaps to modify the tests to pull some local images (fixtures?) from any one of the 3? in-container Docker registries instead of relying on the Docker Hub (which would be nice anyway). Longer-term, we absolutely need to figure this out. I suppose it is easy enough to add (It is also conceptually really awkward; a tagged Docker Reference, even if not retagged, does not specify a unique image, you can get something completely different depending on what destination you want to store the image in. sigh, no helping this now, I suppose.) |
for _, layer := range m.Layers { | ||
blobs = append(blobs, layer.Digest) | ||
} | ||
blobs = append(blobs, m.Config.Digest) |
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 like to sort this out before moving to containers/image - so when a v2s2 manifest is retrieved and we look for layers I'm appending the config digest here because it's needed (as it was in v2s1, but v2s1 had the config embedded into the manifest itself) so now we have to retrieve the config somehow. Would it be better by adding a Config()
to the generalManifest
interface? or is this ok? /cc @mtrmac
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.
nvm tried a fix into containers/image#19
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, see the conversation around containers/image#19.
I'll keep this open until we merge: containers/image#19 after those are merged, I'll split the OCI bits to containers/image and refactor code here |
containers/image#17 / containers/image#19 as it is now does not, AFAICS, fix the integration test failures (pulling s2 from Docker Hub and trying to save it into OpenShift, which only supports s1). |
they don't I'm just tracking progress in containers/image to go forward with this one |
Signed-off-by: Antonio Murdaca <runcom@redhat.com>
right, using images with v2s1 only manifests should fix the test - I just pushed to test my fix out |
right, test passed but we should look forward to fix this as you described in #102 (comment) |
Good to confirm that this is the culprit; if this is fixed, it would be nice to revert to using the |
Right, we still have to fix the underlying issue though :( taking care of this tomorrow probably |
Signed-off-by: Antonio Murdaca <runcom@redhat.com>
|
||
dest, err := parseImageDestination(context, context.Args()[1]) | ||
if err != nil { | ||
return fmt.Errorf("Error initializing %s: %v", context.Args()[1], err) | ||
} | ||
signBy := context.String("sign-by") | ||
src := image.FromSource(rawSource, dest.SupportedImageDestinationMIMEType()) |
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.
Yay, it works! Stylistically it would be a bit more readable to do all of dest
first and source
second, then, so that we have a clear, simple dest
paragraph and src
paragraph.
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.
+1 will do it after we merge containers/image#26 and do the full rebase here
This doesn't do much except 90b7bdd which moves manifest utils to a top level pkg.
The rest implements OCI ImageDestination which basically outputs to a directory following https://github.com/opencontainers/image-spec/blob/master/image-layout.md
Note that the OCI image spec it's still WIP itself.
This is still WIP but I greatly appreciate any comment @mtrmac (there are some ugliness just to make this work so don't bother about them I will fix it of course, I'd like to have an impression on the direction first)
It's already kind of working (expect reference naming which has an issue upstream as well):