Support importing docker images#2633
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2633 +/- ##
==========================================
+ Coverage 44.05% 45.13% +1.08%
==========================================
Files 94 92 -2
Lines 10238 9976 -262
==========================================
- Hits 4510 4503 -7
+ Misses 5008 4754 -254
+ Partials 720 719 -1
Continue to review full report at Codecov.
|
dmcgowan
force-pushed
the
import-docker
branch
from
776f601 to
c473cc0
Compare
|
Simplified the code quite a bit based on Tonis' feedback about only needing to get the layout structure from |
dmcgowan
force-pushed
the
import-docker
branch
from
c473cc0 to
42d5560
Compare
There was a problem hiding this comment.
Do we still want to keep oci importer?
There was a problem hiding this comment.
Maybe not, at first the Docker one was pretty complicated compared to OCI
dmcgowan
force-pushed
the
import-docker
branch
from
42d5560 to
f7d8bec
Compare
|
This PR is depending on #2642 to fix a bug I found while testing this |
dmcgowan
force-pushed
the
import-docker
branch
from
f7d8bec to
c06eed5
Compare
Allow customization of reference creation. Add option for digest references. Signed-off-by: Derek McGowan <[email protected]>
Update ctr to support all formats by default Signed-off-by: Derek McGowan <[email protected]>
Support any layout and rely on manifest.json to reference blobs Signed-off-by: Derek McGowan <[email protected]>
dmcgowan
force-pushed
the
import-docker
branch
2 times, most recently
from
ae8d39c to
25dbce4
Compare
There was a problem hiding this comment.
likely/recommended to contain only tags, but IIUC it is also valid to include the base image name?
There was a problem hiding this comment.
And we should have tests (can be a separate PR)
There was a problem hiding this comment.
Updated to say it may contain it, I think we should be exporting with the base name otherwise there is important information which gets excluded.
There was a problem hiding this comment.
nit: I would expect "bundle" to mean OCI runtime bundle (config.json + rootfs)
Maybe we can call this package just "oci"
There was a problem hiding this comment.
Do we use any other term to represent this tar archive? I don't think oci is the right package name here, it is a subset of the OCI functionality and now includes functionality which is not OCI.
There was a problem hiding this comment.
I am going to just rename it to archive, I was trying to avoid overloading that package name but I think that is the most correct package name, since it is an image archive.
There was a problem hiding this comment.
This ends up importing from github.com/docker/distribution/reference which I want to avoid using
There was a problem hiding this comment.
Should we mention that the default platform will be assigned?
There was a problem hiding this comment.
I was trying to avoid platform related stuff in this PR, platform options should definitely be included as well as the option to not unpack.
dmcgowan
force-pushed
the
import-docker
branch
from
25dbce4 to
9e8d84e
Compare
There was a problem hiding this comment.
The OCI spec says this is REQUIRED https://github.com/opencontainers/image-spec/blame/master/image-layout.md#L24, I'd at least pop out a warning message if it's empty, if not an error.
There was a problem hiding this comment.
It is REQUIRED which is why when it does not exist the content gets interpreted as in the Docker format. I could make this more clear in the code.
There was a problem hiding this comment.
ok.. but which platform? should it be the first or the matching.. hmm
There was a problem hiding this comment.
This comment is trying to say that only a manifest list can resolve to multiple platforms and the descriptor is explicitly not a manifest list. When it is just a manifest, the platform gets resolved from the config which can only have a single platform.
There was a problem hiding this comment.
kk so the > 0 was a no panic check.. should it be a warning or error if > 1 || < 1 here? or is this just an optional overwrite to the default...
There was a problem hiding this comment.
The resolve can be 0 or 1, not too concerned about >1 since the descriptor was created above in a way where it can never be >1
Signed-off-by: Derek McGowan <[email protected]>
Signed-off-by: Derek McGowan <[email protected]>
dmcgowan
force-pushed
the
import-docker
branch
from
9e8d84e to
da6d290
Compare
Codecov Report
@@ Coverage Diff @@
## master #2633 +/- ##
==========================================
+ Coverage 43.88% 44.75% +0.87%
==========================================
Files 94 92 -2
Lines 10307 10164 -143
==========================================
+ Hits 4523 4549 +26
+ Misses 5067 4898 -169
Partials 717 717
Continue to review full report at Codecov.
|
6 participants
Updates the default image importer to support both OCI and docker image formats. OCI is always preferred when provided. This also refactors the import code to return the descriptor for an index which was placed in the content store. The docker import code will create a full OCI index with reference annotations rather than just the manfiests. The client code then creates images from the imported index. By default the index will not be retained and be garbage collected.
Updated tests are WIP, only manually tested. Can add to this PR or follow up.