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.

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

integration-cli: use cmd.Stdin instead of cat/tee for TestExportContainerAndImportImage #10793

Merged
merged 1 commit into from
Feb 16, 2015
Merged

integration-cli: use cmd.Stdin instead of cat/tee for TestExportContainerAndImportImage #10793

merged 1 commit into from
Feb 16, 2015

Conversation

ahmetb
Copy link
Contributor

@ahmetb ahmetb commented Feb 14, 2015

TestExportContainerAndImportImage:

os.Exec("bash", "-c", dockerBinary) ends up making a call like bash -c "c:\...\docker.exe" on windows which doesn't work because bash won't accept windows paths and there's no unix path available for dockerBinary.

This test makes use of exec.Command.Stdin to pass the docker exported image back to docker import using strings.Reader.

  • Upside: fixes the test on windows
  • Downside: cat/tee compatibility of the commands is no longer tested in this test case

Signed-off-by: Ahmet Alp Balkan ahmetalpbalkan@gmail.com
Label: #windows
cc: @tianon @unclejack @jfrazelle @icecrime

…inerAndImportImage

os.Exec("bash", "-c", dockerBinary) ends up making a call like
bash -c c:\...\docker.exe on windows msys shell, which does not work.

This test makes use of exec.Command.Stdin to pass image back to
docker import.

- Upside: fixes the test on windows
- Downside: cat/tee compatibility is no longer tested in this test case

Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
@ahmetb
Copy link
Contributor Author

ahmetb commented Feb 14, 2015

https://jenkins.dockerproject.com/job/Windows-PRs/35/console
=== RUN TestExportContainerAndImportImage
[PASSED]: export - export a container
[PASSED]: import - import an image
--- PASS: TestExportContainerAndImportImage (3.64s) 👍

@icecrime
Copy link
Contributor

Nice! LGTM

@tianon
Copy link
Member

tianon commented Feb 14, 2015

+100

LGTM

IMO, this is how this test (and similar tests) should've worked in the first place. 👍

@duglin
Copy link
Contributor

duglin commented Feb 14, 2015

LGTM
I am curious though about keeping everything in a buffer. At some point, wouldn't using a pipe between the two commands be a more appropriate solution?

@unclejack
Copy link
Contributor

LGTM

@ahmetb
Copy link
Contributor Author

ahmetb commented Feb 14, 2015

IMO, this is how this test (and similar tests) should've worked in the first place.

I thought it was intentional to test how unix tools work with cli. 😮

@duglin valid concern. in this case image is small, so I ignored it, also in this case I'd prefer runCommandWithOutput to return []byte; not string.

@jessfraz
Copy link
Contributor

LGTM

jessfraz pushed a commit that referenced this pull request Feb 16, 2015
…inerAndImportImage-fix

integration-cli: use cmd.Stdin instead of cat/tee for TestExportContainerAndImportImage
@jessfraz jessfraz merged commit 7e9dd94 into moby:master Feb 16, 2015
@ahmetb ahmetb deleted the win-cli/TestExportContainerAndImportImage-fix branch February 16, 2015 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants