Conversation
|
Sorry for the delay in responding to this. Even more apologies if these are topics you've already discussed. 😞 +1 to changing the OCI annotations to match the recommended/reserved standard. I'm slightly -1 to having a "just mutate this manifest" method, since it breaks some invariants we have in place otherwise, and we might find out those invariants are load-bearing. I believe it should be possible to achieve the same result just using I agree that having I'm not totally convinced even this is a good idea, so if we go down this path I might want us to make it a hidden CLI surface, with docs saying that it might go away in the future. In fact, as I'm thinking of it, would a command that does just that be sufficient for your use case, even outside of Let me know what you think. |
I am on the same page with you. I used mutators for everything else except for changing the manifest. the reason i did that is I wanted to preserve the ordering of the layer field. in theory i could have just do removeLayers and appendLayers but i am only interested in some specific layers so I could, in theory, displace the order of a layer and break the image. I didn't realize there was a mutate.ImageManifest.
having --format oci and ocify would be even more confusing IMHO :D. maybe we rename the option to be something more descriptive? i believe options should have been
I feel your pain as a maintainer that you don't feel like landing a feature unless there is a great public interest. I do that myself too. a lot of people really don't care about what mediaTypes their container images have, hell they don't know what it is as long as their deployment works. However, I am under the impression this would useful for others as well and I want to land it here. my use case was to rip out any randomness from the base images. I'll try to enumerate my points 1- docker specification has randomness built-in into their images. it is so easy to break reproducibility and docker helps that with properties such as also given the size of the change, it wouldn't be such a big maintenance burden.
I don't want you to land anything here if you don't feel strongly about it. I am just trying to push crane to a place where you can build images on the disk. (amend RFC). Yet still, if you think my arguments are not good enough, then maybe we could hide this option behind an experimental flag so that people depend on it carefully and beware that it might go away if it is good enough? |
|
you could be anticipating some unforeseen issues with this as well. but if there is anything i can address, i am happy to address them. |
|
I have addressed the issues above. I'd be happy if someone could take another look. I'd really like reach to a conclusion about whether to accept this or not |
|
Hey @thesayyn, sorry I let this linger for so long. I'm definitely still interested in this functionality, there's been a need for it in I'll chat with @jonjohnsonjr about it this week and see if we can make some progress. Thanks for hanging on this long, and thanks for this contribution. |
|
Mixing docker media types definitely should not be prohibited. Maybe we could avoid this case when the image contains layers that don't have OCI counterparts? Or even better, we could have an option to whether hard fail when an image can't be converted to be 100% OCI. |
|
This is something that's also interesting (almost neccessary) for ko-build/ko#738. To provide annotations for the wasi image, OCI format is neccessary and for example quay.io prohibits mixed layers. |
I started working on rules_oci. we kinda need this. This seems to affect other people as well. See project-zot/zot#622 So far the concerns were;
|
90fe63a to
86fb23c
Compare
|
@imjasonh just had another pass at this. removing changes already landed. skimmed down to just oci conversion. FYI. I am getting strange |
|
This Pull Request is stale because it has been open for 90 days with |
| case types.DockerLayer: | ||
| reader, err := layer.Compressed() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("getting layer: %w", err) | ||
| } | ||
| layer, err = tarball.LayerFromOpener(func() (io.ReadCloser, error) { return reader, nil }, tarball.WithMediaType(types.OCILayer)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("building layer: %w", err) | ||
| } | ||
| case types.DockerUncompressedLayer: | ||
| reader, err := layer.Uncompressed() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("getting layer: %w", err) | ||
| } | ||
| layer, err = tarball.LayerFromOpener(func() (io.ReadCloser, error) { return reader, nil }, tarball.WithMediaType(types.OCIUncompressedLayer)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("building layer: %w", err) | ||
| } | ||
| } | ||
| newLayers = append(newLayers, layer) | ||
| } |
There was a problem hiding this comment.
I suspect there is a better way to do this, this is rather expensive.
There was a problem hiding this comment.
I'd expect that but i wasn't able to find it.
There was a problem hiding this comment.
ping @jonjohnsonjr do you have suggestions to make this less expensive? This PR seems stuck.
There was a problem hiding this comment.
This is annoying because of how partial.Descriptor works, but I would probaby create a new type, something like:
type layerWithMediaType struct {
v1.Layer
mt types.MediaType
}
func (l *layerWithMediaType) Descriptor() (*v1.Descriptor, error) {
// call partial.Descriptor on wrapped layer and update MediaType
}
func (l *layerWithMediaType) MediaType() (types.MediaType, error) {
return l.mt, nil
}|
This Pull Request is stale because it has been open for 90 days with |
|
Not stale |
Implements #1245. cc @jonjohnsonjr
Open to any kind of suggestions. I did my best to keep the patch as small as possible.
I will add tests after i address the changes.
the patch in action: thesayyn/oci-image:latest-crane pulled debian and pushed via crane.