runtime: Add ManifestList to LookupImageOptions
#747
Conversation
libimage/runtime.go
Outdated
| // If matching images resolves to a manifest list, return manifest list | ||
| // instead of resolving to image instance, if manifest list is not found | ||
| // try resolving image. | ||
| ReturnManifestIfPresent bool |
There was a problem hiding this comment.
Why not just make lookupManifest public?
There was a problem hiding this comment.
@vrothberg I did that first but realized that lookupManifest is being used by only manifest commands which are supposed to fail if no manifest are found but we need a flag which first looks for manifest and return manifest if found otherwise start looking for image.
There was a problem hiding this comment.
Very fair point, thanks for clarifying. I should have checked more carefully :^)
There was a problem hiding this comment.
Sorry for the late request: Could we rename the option to just ManifestList? LookupImageOptions{ManifestList: true} seems quite intuitive and much shorter.
There was a problem hiding this comment.
@vrothberg sure can do that, should we make it ManifestListIfPresent ? Since it returns manifest list if present otherwise returns image
There was a problem hiding this comment.
I prefer without IfPresent. It feels intuitive that it would resolve to a manifest list only if it's a manifest list. The comment goes into the details in case users wonder.
There was a problem hiding this comment.
@vrothberg cool I agree, we can always refer to comments.
There was a problem hiding this comment.
Could you add a regression test here as well?
Look at the _test.go files in ./libimage. I suggest to create manifest list and add busybox to it. Then, tag the manifest list and check whether it's doing the right thing. This way we'll make sure to regress on it in the future.
|
LGTM |
|
@vrothberg you want this test in containers/common on in buildah? |
I want the test here. It's painful to catch libimage bugs in buildah or podman, so I want to add tests if it's possible. In this case, tests are possible. |
|
No issues I'll add a tests here. |
flouthoc
force-pushed
the
return-manifest-if-present
branch
7 times, most recently
from
cd61602 to
621c114
Compare
|
@rhatdan @vrothberg added requested tests, PTAL |
|
LGTM |
libimage/runtime.go
Outdated
| // If matching images resolves to a manifest list, return manifest list | ||
| // instead of resolving to image instance, if manifest list is not found | ||
| // try resolving image. | ||
| ReturnManifestIfPresent bool |
There was a problem hiding this comment.
Sorry for the late request: Could we rename the option to just ManifestList? LookupImageOptions{ManifestList: true} seems quite intuitive and much shorter.
libimage/manifest_list_test.go
Outdated
| require.NotNil(t, list) | ||
|
|
||
| manifestListOpts := &ManifestListAddOptions{All: true} | ||
| _, err = list.Add(ctx, "docker://fedora", manifestListOpts) |
There was a problem hiding this comment.
Please use busybox instead of fedora. It is much smaller and hence reduces the cost of CI.
If matching images resolves to a manifest list, return manifest list instead of resolving to image instance, if manifest list is not found try resolving image. Signed-off-by: Aditya Rajan <[email protected]>
flouthoc
force-pushed
the
return-manifest-if-present
branch
from
621c114 to
0f93c92
Compare
flouthoc
changed the title
ReturnManifestIfPresent to LookupImageOptionsManifestList to LookupImageOptions
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, vrothberg The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
If matching images resolves to a manifest list, return manifest list
instead of resolving to image instance, if manifest list is not found
try resolving image.