Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remote copy #8747

Merged
merged 2 commits into from
Dec 18, 2020
Merged

remote copy #8747

merged 2 commits into from
Dec 18, 2020

Conversation

vrothberg
Copy link
Member

@vrothberg vrothberg commented Dec 16, 2020 •
edited
Loading

Implement podman-remote cp and break out the logic from the previously
added pkg/copy into it's basic building blocks and move them up into
the ContainerEngine interface and cmd/podman.

The --pause flag is now deprecated and turned into a nop.

Note that this commit is vendoring a non-release version of Buildah to
pull in updates to the copier package.

Signed-off-by: Valentin Rothberg rothberg@redhat.com

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Dec 16, 2020
@vrothberg
Copy link
Member Author

One remaining bug that I will debug with Nalin.

@vrothberg vrothberg changed the title WIP - remote copy remote copy Dec 17, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 17, 2020
rhatdan
rhatdan approved these changes Dec 17, 2020
View reviewed changes
Copy link
Member

@rhatdan rhatdan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Impressive work
@containers/podman-maintainers PTAL

cmd/podman/containers/cp.go Show resolved Hide resolved
pkg/domain/infra/abi/containers_stat.go Outdated Show resolved Hide resolved
@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@baude
Copy link
Member

baude commented Dec 17, 2020

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 17, 2020
@baude
Copy link
Member

baude commented Dec 17, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2020
edsantiago
edsantiago reviewed Dec 17, 2020
View reviewed changes
Copy link
Member

@edsantiago edsantiago left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, nice work, but there's one broken test and very weird (unexpected) behavior.

test/system/065-cp.bats Outdated Show resolved Hide resolved
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2020
@vrothberg
Copy link
Member Author

Repushed. Now both, --pause and --export, are deprecated and turned into nops. I didn't use cobra's MarkDeprecated as it's printing the warning twice for rootless podman due to the reexec.

TomSweeneyRedHat
TomSweeneyRedHat reviewed Dec 17, 2020
View reviewed changes
cmd/podman/containers/cp.go
return errors.Errorf("container %q does not exist", container)
}
return nil
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I have the exists() function from the new API stuck in my head, but I thought we'd similar functionality elsewhere in libpod? I looked but didn't find it quickly. This feels like a function that should live in util.

This is fine here for now, but something to ruminate on.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there's a ContainerExists function. I thought to put that into a dedicated function (plus the checks) to consolidate the logic a bit further.

@TomSweeneyRedHat
Copy link
Member

Changes LGTM
One question for consideration and unfortunately a lot of red in the tests.

@vrothberg
Copy link
Member Author

Changes LGTM

Thanks for checking!

One question for consideration and unfortunately a lot of red in the tests.

Should turn green now. Removing the options made the help checks fail (as there are no options any more).

@rhatdan
Copy link
Member

rhatdan commented Dec 17, 2020

/lgtm
/hold
Go on vacation now...

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 17, 2020
@vrothberg
Copy link
Member Author

Go on vacation now...

One more day to go :)

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2020
@vrothberg
Copy link
Member Author

One more time ... Ubuntu remote tests failed. Rebased.

@vrothberg vrothberg mentioned this pull request Dec 18, 2020
Closed
@vrothberg
Copy link
Member Author

Note to self: close #4207 once this is merged

@vrothberg
remote copy
adcb3a7 
Implement `podman-remote cp` and break out the logic from the previously
added `pkg/copy` into it's basic building blocks and move them up into
the `ContainerEngine` interface and `cmd/podman`.

The `--pause` and `--extract` flags are now deprecated and turned into
nops.

Note that this commit is vendoring a non-release version of Buildah to
pull in updates to the copier package.

Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
@vrothberg vrothberg mentioned this pull request Dec 18, 2020
Closed
@vrothberg
Copy link
Member Author

Tried to reproduce in the Google Cloud but it worked 🙉 Let's pray to the CI gods.

@vrothberg
Copy link
Member Author

Oh man, that's frustrating.

@vrothberg
system tests: set PODMAN_TIMEOUT to 120
641272d 
The new Ubuntu 20.04 VMs seem very slow and fail reproducibly in a build
test (i.e, "wordir, cmd, env, label").  Bumping up the time out to 120
seconds will help get the CI green.

See github.com/containers/pull/8747.

Signed-off-by: Valentin Rothberg <rothberg@redhat.com>
@vrothberg vrothberg mentioned this pull request Dec 18, 2020
Closed
@baude
Copy link
Member

baude commented Dec 18, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 18, 2020
@baude
Copy link
Member

baude commented Dec 18, 2020

congrats @vrothberg

@vrothberg
Copy link
Member Author

/hold cancel

@openshift-ci-robot 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 18, 2020
@openshift-merge-robot openshift-merge-robot merged commit 5c6b5ef into containers:master Dec 18, 2020
@vrothberg vrothberg deleted the run-950 branch December 18, 2020 15:16
@edsantiago edsantiago mentioned this pull request Dec 18, 2020
Closed
@edsantiago edsantiago mentioned this pull request Jan 4, 2021
Closed
@edsantiago edsantiago mentioned this pull request Jan 19, 2021
Closed
@github-actions 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 github-actions bot locked as resolved and limited conversation to collaborators Sep 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@edsantiago edsantiago edsantiago left review comments

@rhatdan rhatdan rhatdan approved these changes

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@Luap99 Luap99 Luap99 left review comments

Assignees

@rhatdan rhatdan

@baude baude

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@vrothberg @openshift-ci-robot @baude @TomSweeneyRedHat @rhatdan @edsantiago @Luap99 @openshift-merge-robot