Fixes podman-remote save to directories does not work #10025

zhangguanzhang · 2021-04-14T12:10:06Z

Fixes: #9742
Signed-off-by: zhangguanzhang [email protected]

openshift-ci-robot · 2021-04-14T12:10:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: zhangguanzhang

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~~ [zhangguanzhang]

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

rhatdan · 2021-04-14T13:09:20Z

pkg/api/handlers/libpod/images.go

@@ -244,6 +244,7 @@ func ExportImages(w http.ResponseWriter, r *http.Request) {
 	decoder := r.Context().Value("decoder").(*schema.Decoder)
 	query := struct {
 		Compress   bool     `schema:"compress"`
+		Remote     bool     `schema:"remote"`


Why is this needed. It is Remote by definition?

rhatdan · 2021-04-14T13:12:15Z

pkg/api/handlers/libpod/images.go

+	// client will unArchive after receive the archive file
+	// so must convert is at here
+	if query.Remote {
+		if query.Format == define.OCIManifestDir {


Would it be better to just do this on the client side and not change the server side at all.

Server will save to dir, not respone in on file.and client will untar the archive file

I am saying that the client checks to see if it is remote and sends across the

query.Format = define.OCIArchive or query.Format = define.V2s2Archive

And then dumps it locally.

I am saying that the client checks to see if it is remote and sends across the

query.Format = define.OCIArchive or query.Format = define.V2s2Archive

And then dumps it locally.

if format is an archive, will return at these lines (end at L279)

podman/pkg/domain/infra/tunnel/images.go

Lines 270 to 280 in df6c7c2

exErr := images.Export(ir.ClientCtx, append([]string{nameOrID}, tags...), f, options)

if err := f.Close(); err != nil {

return err

}

if exErr != nil {

return exErr

}

if opts.Format != "oci-dir" && opts.Format != "docker-dir" {

return nil

}

if is a dir, will recreive an archive of the dir, and untar at this line

podman/pkg/domain/infra/tunnel/images.go

Line 299 in df6c7c2

return utils2.UntarToFileSystem(opts.Output, f, nil)

The logic already exists, so I don’t need to add it

vrothberg · 2021-04-15T06:37:00Z

pkg/bindings/images/images.go

@@ -143,6 +143,7 @@ func Export(ctx context.Context, nameOrIDs []string, w io.Writer, options *Expor
 	for _, ref := range nameOrIDs {
 		params.Add("references", ref)
 	}
+	params.Add("remote", "true")


This parameter shouldn't be needed as it's always remote.

Should I remove all the remote parameter in all files?

Thanks, yes please remove it.

vrothberg · 2021-04-15T11:19:19Z

pkg/api/handlers/libpod/images.go

+	}
+	if query.Format == define.V2s2ManifestDir {
+		query.Format = define.V2s2Archive
+	}


Non-blocking: a switch query.Format {...} may be more idiomatic.

LGTM

Signed-off-by: zhangguanzhang <[email protected]>

TomSweeneyRedHat · 2021-04-16T22:04:43Z

LGTM
and happy test buttons

rhatdan · 2021-04-17T10:33:05Z

Someone will have to explain to me why this is being done on the server side? Seems like the client knows what it is doing, and the server is not doing what the caller requested. If I write a python client and ask for a directory, I will not have the majic available to convert, and will end up with a mess. Or am I missing something?

zhangguanzhang · 2021-04-17T10:53:53Z

Someone will have to explain to me why this is being done on the server side? Seems like the client knows what it is doing, and the server is not doing what the caller requested. If I write a python client and ask for a directory, I will not have the majic available to convert, and will end up with a mess. Or am I missing something?

server side always return file bytes in one file：

podman/pkg/api/handlers/libpod/images.go

Lines 230 to 236 in 6ff56ab

    
           rdr, err := os.Open(output) 
        
           if err != nil { 
        
           	utils.Error(w, "Something went wrong.", http.StatusInternalServerError, errors.Wrap(err, "failed to read the exported tarfile")) 
        
           	return 
        
           } 
        
           defer rdr.Close() 
        
           utils.WriteResponse(w, http.StatusOK, rdr)

rhatdan · 2021-04-19T14:21:59Z

Well outvoted and it does fix a bug, so merging.
/lgtm

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 14, 2021

rhatdan reviewed Apr 14, 2021

View reviewed changes

zhangguanzhang force-pushed the remote-save-dir branch from 316f0a1 to 0f9d3a9 Compare April 15, 2021 01:28

zhangguanzhang requested a review from vrothberg April 15, 2021 03:18

vrothberg reviewed Apr 15, 2021

View reviewed changes

zhangguanzhang force-pushed the remote-save-dir branch 2 times, most recently from 97229a1 to 0ad20f7 Compare April 15, 2021 08:12

vrothberg reviewed Apr 15, 2021

View reviewed changes

Fixes podman-remote save to directories does not work

21827f6

Signed-off-by: zhangguanzhang <[email protected]>

zhangguanzhang force-pushed the remote-save-dir branch from 0ad20f7 to 21827f6 Compare April 15, 2021 11:24

openshift-ci-robot assigned rhatdan Apr 19, 2021

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 19, 2021

openshift-merge-robot merged commit a917bae into containers:master Apr 19, 2021

edsantiago mentioned this pull request Apr 21, 2021

compose flake: rootlesskit port forwarder not setup correctly #10052

Closed

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 23, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes podman-remote save to directories does not work #10025

Fixes podman-remote save to directories does not work #10025

zhangguanzhang commented Apr 14, 2021

openshift-ci-robot commented Apr 14, 2021

rhatdan Apr 14, 2021

rhatdan Apr 14, 2021

zhangguanzhang Apr 14, 2021

rhatdan Apr 14, 2021

zhangguanzhang Apr 15, 2021 •

edited

Loading

vrothberg Apr 15, 2021

zhangguanzhang Apr 15, 2021

vrothberg Apr 15, 2021

vrothberg Apr 15, 2021

zhangguanzhang Apr 15, 2021

TomSweeneyRedHat commented Apr 16, 2021

rhatdan commented Apr 17, 2021

zhangguanzhang commented Apr 17, 2021

rhatdan commented Apr 19, 2021

	exErr := images.Export(ir.ClientCtx, append([]string{nameOrID}, tags...), f, options)
	if err := f.Close(); err != nil {
	return err
	}
	if exErr != nil {
	return exErr
	}

	if opts.Format != "oci-dir" && opts.Format != "docker-dir" {
	return nil
	}

Fixes podman-remote save to directories does not work #10025

Fixes podman-remote save to directories does not work #10025

Conversation

zhangguanzhang commented Apr 14, 2021

openshift-ci-robot commented Apr 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhangguanzhang Apr 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Apr 16, 2021

rhatdan commented Apr 17, 2021

zhangguanzhang commented Apr 17, 2021

rhatdan commented Apr 19, 2021

zhangguanzhang Apr 15, 2021 •

edited

Loading