-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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.
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
podman cp: big set of system tests #3887
Conversation
podman cp has had some unexpected bugs, and still has some surprising behavior. It looks like this part of the code is fragile. Add tests to try to prevent future breakages. Note that two of the new tests are disabled (skipped) until containers#3829 gets fixed. Signed-off-by: Ed Santiago <[email protected]>
Please note that the "weird symlink expansion" test elicits what I believe to be incorrect behavior. I'm getting more and more tempted to file an issue on it. |
LGTM |
/approve |
LGTM! Many thanks Ed. |
(Maybe we should have @jwhonce cherry-pick this on top of the symlink globbing PR before we merge it, to make sure everything passes there?) |
Thanks @mheon and @rhatdan. Could I pretty-please ask someone for an opinion on that "weird" test case though? Should I file an issue?
Jhon's PR addresses one of the skipped tests, but not (last time I checked) the other one. I think that was a difficult one. |
} | ||
|
||
|
||
function teardown() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if we have one, but if not, could you add a test where you copy a file from the host and the target is a symlink in the container? The file is then visible at the symlinks target in the container, but is not seen on the host? Especially in the corresponding directory on the host? i.e symlink in the container for /test->//tmp then after 'cp mytestfile ctr:/test', the mytestfile doesn't show up in /tmp on the host too (or instead).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is two of the three paths in the "weird" test
LGTM |
LGTM and it looks like @edsantiago opened an issue for the findings he was questioning. I think that's the right approach. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
I love the comments in the tests. It's a great example how future tests can look like 👍
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, mheon, 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 |
/lgtm |
podman cp has had some unexpected bugs, and still has
some surprising behavior. It looks like this part of
the code is fragile. Add tests to try to prevent
future breakages.
Note that two of the new tests are disabled (skipped)
until #3829 gets fixed.
Signed-off-by: Ed Santiago [email protected]