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: readContainerFileWithExec for links tests #10837

Merged
merged 2 commits into from Feb 24, 2015

Conversation

Projects
None yet
7 participants
@ahmetb
Contributor

ahmetb commented Feb 16, 2015

Shout out to @estesp for the idea. Some use cases of
readContainerFile can be replaced with docker exec $id cat $file.
This helper method can eliminate the assumption that
host/cli should be on the same machine, partially.

TestRunMutableNetworkFiles and TestRunResolvconfUpdater still
need to access the docker host filesystem as they modify
the file directly from there assuming cli and daemon are
on the same machine. They may keep usingreadContainerFile.

This fixes TestLinksUpdateOnRestart and TestLinksHostsFilesInject
for Windows CI.

Signed-off-by: Ahmet Alp Balkan ahmetalpbalkan@gmail.com
Label: #windows
Cc: @estesp @duglin @unclejack @jfrazelle @tianon

@ahmetb ahmetb changed the title from readContainerFileWithExec for links tests to integration-cli: readContainerFileWithExec for links tests Feb 16, 2015

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 16, 2015

https://jenkins.dockerproject.com/job/Windows-PRs/46/console
=== RUN TestLinksUpdateOnRestart
[PASSED]: link - ensure containers hosts files are updated on restart
=== RUN TestLinksHostsFilesInject
[PASSED]: link - ensure containers hosts files are updated with the link alias. 👍

@jessfraz

This comment has been minimized.

Contributor

jessfraz commented Feb 16, 2015

this is going to break lxc, can we move all the exec tests to a certain file that wont run on lxc, see the exec_test file for example

@jessfraz

This comment has been minimized.

Contributor

jessfraz commented Feb 16, 2015

or we need a way to do this without exec

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 16, 2015

Or we may skip these altogether or move to "//+build daemon" as it's not really testing the CLI but daemon.

@cpuguy83

This comment has been minimized.

Contributor

cpuguy83 commented Feb 16, 2015

Or use POST /contaienrs/<id>/copy

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 16, 2015

@cpuguy83 erm, you're right... @jfrazelle how's that sound to you?

@jessfraz

This comment has been minimized.

Contributor

jessfraz commented Feb 16, 2015

sgtm

@LK4D4

This comment has been minimized.

Contributor

LK4D4 commented Feb 20, 2015

yeah, seems like copy is best option (apart from dropping lxc support)

@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 20, 2015

@jfrazelle @LK4D4 I pretty much implemented the docker cp support except it appears that we can't cp "/etc/hosts" from a container #9998, #10037. @tianon

How about we skip these 2 test cases with testRequires(t, ExecSupport) and keep the original "docker exec .. cat /file" implementation?

(I guess we can use test_no_exec go build tag to find out if execdriver==lxc and create 2 files test_vars_exec.go, test_vars_noexec.go and add supportsExec const for testRequires? @jfrazelle)

@LK4D4

This comment has been minimized.

Contributor

LK4D4 commented Feb 21, 2015

+1 for supportExec

ahmetb added some commits Feb 16, 2015

readContainerFileWithExec for links tests
Shout out to @estesp for the idea. Some use cases of
`readContainerFile` can be replaced with `docker exec $id cat $file`.
This helper method can eliminate the requirement that
host/cli should be on the same machine.

TestRunMutableNetworkFiles and TestRunResolvconfUpdater still
need to access the docker host filesystem as they modify
the file directly from there assuming cli and daemon are
on the same machine.

This fixes TestLinksUpdateOnRestart and TestLinksHostsFilesInject
for Windows CI.

Signed-off-by: Ahmet Alp Balkan <ahmetalpbalkan@gmail.com>
integration-cli: add test requirement ExecSupport
Skip tests based on remote daemon's exec support (to exclude
these tests from `make test` ran in LXC case). Makes use of
`test_no_exec` build tag passed by build scripts.

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

This comment has been minimized.

Contributor

ahmetb commented Feb 21, 2015

@LK4D4 @cpuguy83 @jfrazelle I just took the fixes.

  • Made use of test_no_exec build tag to skip the tests in LXC case (102e061)
  • implemented readContainerFileWithExec (6bbb456).
@jessfraz

This comment has been minimized.

Contributor

jessfraz commented Feb 23, 2015

yayyy thanks @ahmetalpbalkan LGTM

@jessfraz

This comment has been minimized.

Contributor

jessfraz commented Feb 24, 2015

ping @LK4D4

@crosbymichael

This comment has been minimized.

Member

crosbymichael commented Feb 24, 2015

LGTM

crosbymichael added a commit that referenced this pull request Feb 24, 2015

Merge pull request #10837 from ahmetalpbalkan/win-cli/readContainerFi…
…le-with-exec

integration-cli: readContainerFileWithExec for links tests

@crosbymichael crosbymichael merged commit 435d654 into moby:master Feb 24, 2015

1 check passed

janky Jenkins build Docker-PRs 1550 has succeeded
Details
@ahmetb

This comment has been minimized.

Contributor

ahmetb commented Feb 24, 2015

yaay, thanks. 😂

@ahmetb ahmetb deleted the ahmetb:win-cli/readContainerFile-with-exec branch Feb 24, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment