Drop name argument from Load API #8112

QiWang19 · 2020-10-22T19:07:17Z

Drop the name argument to Load API. Splitting the code for load single and multiple images into two functions, LoadImageFromSingleImageArchive LoadAllImageFromArchive
Specify in the document the usage of the optional name argument of the podman load is tagging an additional image.
Close #7337

// docker-archive and oci-archive
$ podman load -i ap2.tar f00
Getting image source signatures
Copying blob 364dcac827e6 done  
Copying config 9785808968 done  
Writing manifest to image destination
Storing signatures
Loaded image(s): docker.io/library/alpine
$ podman images
REPOSITORY                TAG     IMAGE ID      CREATED     SIZE
docker.io/library/alpine  latest  9785808968ba  7 days ago  5.84 MB
localhost/f00             latest  9785808968ba  7 days ago  5.84 MB

Signed-off-by: Qi Wang [email protected]

rhatdan · 2020-10-22T19:29:59Z

LGTM

libpod/runtime_img.go

jwhonce · 2020-10-27T17:28:19Z

/lgtm

vrothberg

I want a head nod from @mtrmac on it. The code is tricky and my eyes are tired :)

vrothberg · 2020-10-27T17:57:50Z

docs/source/markdown/podman-load.1.md


 ## DESCRIPTION
 **podman load** loads an image from either an **oci-archive** or a **docker-archive** stored on the local machine into container storage. **podman load** reads from stdin by default or a file if the **input** option is set.
-You can also specify a name for the image if the archive does not contain a named reference, of if you want an additional name for the local image.


I think we should keep that but document which formats support it.

vrothberg · 2020-10-27T17:59:10Z

libpod/runtime_img.go

-			if !strings.Contains(name, "/") {
-				return layout.NewReference(inputFile, fmt.Sprintf("%s/%s", image.DefaultLocalRegistry, name))
+			if name != "" {
+				return nil, errors.Errorf("oci format does not support additional name %s", name)


That seems unrelated. Why's that needed?

vrothberg · 2020-10-27T18:00:09Z

/hold

rhatdan · 2020-10-27T18:00:17Z

/approve

openshift-ci-robot · 2020-10-27T18:00:20Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: QiWang19, rhatdan

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:

~~OWNERS~~ [rhatdan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

mtrmac

A quick impression is that this makes sense, still I’d like this PR to go further:

More extensive refactoring: AFAICS the name parameter is now only used for a comparison with "" (and an error message that is probably now very misleading and should be updated). So, should the callers be passing it in at all? The name parameter either shouldn’t exist, or it should exist and be used, or it should be replaced by a boolean (or possibly by splitting this function into two, LoadImageFromSingleImageArchive vs. LoadAllImageFromArchive, or whatever is best for the callers). And so on all the way up the call stack, including possibly dropping the parameter from the API.
Documentation: While this probably resolves the ambiguity in actual behavior, will it be clear to future readers, and API users? What are we doing here, what does the name field mean after all? (Its mere presence seems to have a non-obvious effect for docker-archive; and in the other formats we just don’t allow it instead of having a similar non-obvious effect. Is that right?) fix podman load oci-archive's tar with name #7386 (review) is not quite the level of detail needed here because the code does make more sense, but still broadly those categories of things would be nice to record for posterity somewhere — at least for the API it should be clearly documented what the name/tag parameters mean. (Or if the parameters are removed, there will be nothing left to document, sure.)

To make sure it is not forgotten, #7386 (review) says this should be dropped in Podman 3.0. Is this the right time/branch?

mtrmac · 2020-10-27T19:02:17Z

libpod/runtime_img.go

-			if !strings.Contains(name, "/") {
-				return ociarchive.NewReference(inputFile, fmt.Sprintf("%s/%s", image.DefaultLocalRegistry, name))
+			if name != "" {
+				return nil, errors.Errorf("oci format does not support additional name %s", name)


BTW this will never be seen by users, the way err is currently maintained by the loop. So the UI might need a bit more thought (or perhaps just give up and return nil, nil here — probably not, but that does depend on what the refactoring in callers ends up doing).

QiWang19 · 2020-10-27T19:28:56Z

To make sure it is not forgotten, #7386 (review) says this should be dropped in Podman 3.0. Is this the right time/branch?

This branch is not meant to drop it. This patch wants to keep the support for docker-archives but removes the description from the document before Podman 3.0.

mtrmac · 2020-10-29T17:39:14Z

This branch is not meant to drop it. This patch wants to keep the support for docker-archives but removes the description from the document before Podman 3.0.

Ah, right, that comment was about removing the “tag the result with $this name” parameter, while this removes the “load $this image from an OCI archive” functionality. Still, is that OK to do before 3.0? Or was it completely broken already?

QiWang19 · 2020-10-29T19:20:11Z

@mtrmac PTAL

QiWang19 · 2020-11-03T15:33:01Z

@mtrmac PTAL

QiWang19 · 2020-11-13T18:33:03Z

@containers/podman-maintainers PTAL

rhatdan · 2020-11-14T10:58:55Z

LGTM, but this is API Breakage so it has to wait til we branche for 3.0.

rhatdan · 2020-12-02T00:01:41Z

@QiWang19 Please rebase and we can merge.

Not pass the name argument to Load API. Specify in the document the usage of the optional argument is tagging an additional image. Close containers#7337 Signed-off-by: Qi Wang <[email protected]>

QiWang19 · 2020-12-02T16:47:01Z

@containers/podman-maintainers PTAL

rhatdan · 2020-12-02T20:33:26Z

/lgtm

rhatdan · 2020-12-02T20:33:39Z

/hold cancel

Looks like containers#7337 was fixed (by containers#8112). Reenable a disabled test for it; and make it actually work. Confirmed that newly-added test fails on d456765 (the commit before containers#8112). Signed-off-by: Ed Santiago <[email protected]>

QiWang19 force-pushed the load-optional-name branch from 1523cff to c2480c2 Compare October 22, 2020 19:10

TomSweeneyRedHat reviewed Oct 22, 2020

View reviewed changes

libpod/runtime_img.go Outdated Show resolved Hide resolved

TomSweeneyRedHat reviewed Oct 22, 2020

View reviewed changes

libpod/runtime_img.go Outdated Show resolved Hide resolved

QiWang19 force-pushed the load-optional-name branch from c2480c2 to f008f36 Compare October 22, 2020 22:40

openshift-ci-robot assigned jwhonce Oct 27, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 27, 2020

vrothberg reviewed Oct 27, 2020

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 27, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 27, 2020

mtrmac reviewed Oct 27, 2020

View reviewed changes

QiWang19 force-pushed the load-optional-name branch from f008f36 to d895f61 Compare October 29, 2020 18:51

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 29, 2020

QiWang19 force-pushed the load-optional-name branch from d895f61 to 56a5f86 Compare October 29, 2020 18:56

QiWang19 changed the title ~~Skip loading oci image with name arg~~ Do not pass name argument to Load API Oct 29, 2020

QiWang19 force-pushed the load-optional-name branch from 56a5f86 to 0df78a7 Compare October 29, 2020 19:00

QiWang19 changed the title ~~Do not pass name argument to Load API~~ Drop name argument from Load API Nov 2, 2020

rhatdan added the 3.0 Features label Nov 18, 2020

Do not pass name argument to Load API

f525d8b

Not pass the name argument to Load API. Specify in the document the usage of the optional argument is tagging an additional image. Close containers#7337 Signed-off-by: Qi Wang <[email protected]>

QiWang19 force-pushed the load-optional-name branch from 0df78a7 to f525d8b Compare December 2, 2020 14:25

openshift-ci-robot assigned rhatdan Dec 2, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 2, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 2, 2020

openshift-merge-robot merged commit e74072e into containers:master Dec 2, 2020

edsantiago mentioned this pull request Dec 2, 2020

BATS: add new load test #8564

Merged

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop name argument from Load API #8112

Drop name argument from Load API #8112

QiWang19 commented Oct 22, 2020 •

edited

Loading

rhatdan commented Oct 22, 2020

jwhonce commented Oct 27, 2020

vrothberg left a comment

vrothberg Oct 27, 2020

vrothberg Oct 27, 2020

vrothberg commented Oct 27, 2020

rhatdan commented Oct 27, 2020

openshift-ci-robot commented Oct 27, 2020

mtrmac left a comment •

edited

Loading

mtrmac Oct 27, 2020

QiWang19 commented Oct 27, 2020

mtrmac commented Oct 29, 2020

QiWang19 commented Oct 29, 2020

QiWang19 commented Nov 3, 2020

QiWang19 commented Nov 13, 2020

rhatdan commented Nov 14, 2020

rhatdan commented Dec 2, 2020

QiWang19 commented Dec 2, 2020

rhatdan commented Dec 2, 2020

rhatdan commented Dec 2, 2020

Drop name argument from Load API #8112

Drop name argument from Load API #8112

Conversation

QiWang19 commented Oct 22, 2020 • edited Loading

rhatdan commented Oct 22, 2020

jwhonce commented Oct 27, 2020

vrothberg left a comment

Choose a reason for hiding this comment

vrothberg Oct 27, 2020

Choose a reason for hiding this comment

vrothberg Oct 27, 2020

Choose a reason for hiding this comment

vrothberg commented Oct 27, 2020

rhatdan commented Oct 27, 2020

openshift-ci-robot commented Oct 27, 2020

mtrmac left a comment • edited Loading

Choose a reason for hiding this comment

mtrmac Oct 27, 2020

Choose a reason for hiding this comment

QiWang19 commented Oct 27, 2020

mtrmac commented Oct 29, 2020

QiWang19 commented Oct 29, 2020

QiWang19 commented Nov 3, 2020

QiWang19 commented Nov 13, 2020

rhatdan commented Nov 14, 2020

rhatdan commented Dec 2, 2020

QiWang19 commented Dec 2, 2020

rhatdan commented Dec 2, 2020

rhatdan commented Dec 2, 2020

QiWang19 commented Oct 22, 2020 •

edited

Loading

mtrmac left a comment •

edited

Loading