Cleanup and merge autotest wrappers.

Cleanup and merge autotest wrappers.

[ericli, 2010-01-25 23:04:28]

http://codereview.chromium.org/546137/diff/1/11
File src/scripts/remote_access.sh (right):

http://codereview.chromium.org/546137/diff/1/11#newcode17
src/scripts/remote_access.sh:17: REMOTE_OUT=$(ssh -o StrictHostKeyChecking=no -o
\
You dont need those flags any more, they were pushed into autotest server
already.

http://git.chromium.org/cgi-bin/gitweb.cgi?p=autotest.git;a=blob;f=server/hos...

line 15.

suggest you to simplify it.

http://codereview.chromium.org/546137/diff/1/13
File src/scripts/run_remote_tests.sh (right):

http://codereview.chromium.org/546137/diff/1/13#newcode16
src/scripts/run_remote_tests.sh:16: assert_inside_chroot
I think you guys do not want to run it inside chroot? what is the deal here?

[seano, 2010-01-25 23:16:57]

http://codereview.chromium.org/546137/diff/1/11
File src/scripts/remote_access.sh (right):

http://codereview.chromium.org/546137/diff/1/11#newcode17
src/scripts/remote_access.sh:17: REMOTE_OUT=$(ssh -o StrictHostKeyChecking=no -o
\
On 2010/01/25 23:04:29, ericli wrote:
> You dont need those flags any more, they were pushed into autotest server
> already.
> 
>
http://git.chromium.org/cgi-bin/gitweb.cgi?p=autotest.git;a=blob;f=server/hos...
> 
> line 15.
> 
> suggest you to simplify it.

Done.

http://codereview.chromium.org/546137/diff/1/13
File src/scripts/run_remote_tests.sh (right):

http://codereview.chromium.org/546137/diff/1/13#newcode16
src/scripts/run_remote_tests.sh:16: assert_inside_chroot
On 2010/01/25 23:04:29, ericli wrote:
> I think you guys do not want to run it inside chroot? what is the deal here?

No, we have to now and for the forseeable future IMO.

[kmixter1, 2010-01-25 23:27:19]

http://codereview.chromium.org/546137/diff/1/10
File src/scripts/mod_for_test_scripts/ssh_keys/testing_rsa (right):

http://codereview.chromium.org/546137/diff/1/10#newcode2
src/scripts/mod_for_test_scripts/ssh_keys/testing_rsa:2:
MIIEoAIBAAKCAQEAvsNpFdK5lb0GfKx+FgsrsM/2+aZVFYXHMPdvGtTz63ciRhq0
Did you remove this key from its old location?  Maybe you did and I just can't
tell from rietveld.

http://codereview.chromium.org/546137/diff/1/11
File src/scripts/remote_access.sh (left):

http://codereview.chromium.org/546137/diff/1/11#oldcode31
src/scripts/remote_access.sh:31: chmod 0400 $TMP_PRIVATE_KEY
What if a different --private_key is passed to the script - in that case keeping
around the setting of the mode to 0400 seems like the right thing to do.

http://codereview.chromium.org/546137/diff/1/11
File src/scripts/remote_access.sh (right):

http://codereview.chromium.org/546137/diff/1/11#newcode17
src/scripts/remote_access.sh:17: REMOTE_OUT=$(ssh -o StrictHostKeyChecking=no -o
\
If you remove this, please verify that image_to_live.sh continues to function
when reimaging a machine with a new image (in which case the host image key
changes).  I only needed this originally exactly for when the host key changes
(which happens every time build_image is run).

On 2010/01/25 23:16:57, seano wrote:
> On 2010/01/25 23:04:29, ericli wrote:
> > You dont need those flags any more, they were pushed into autotest server
> > already.
> > 
> >
>
http://git.chromium.org/cgi-bin/gitweb.cgi?p=autotest.git;a=blob;f=server/hos...
> > 
> > line 15.
> > 
> > suggest you to simplify it.
> 
> Done.

http://codereview.chromium.org/546137/diff/1/13
File src/scripts/run_remote_tests.sh (right):

http://codereview.chromium.org/546137/diff/1/13#newcode16
src/scripts/run_remote_tests.sh:16: assert_inside_chroot
I hope not.  I definitely don't run it inside the chroot now and prefer to
always avoid requiring folks to run anything inside the chroot (would even
prefer creating an outer script that enter_chroot's an inner script).

Also, for readability, please put anything that's not a source (.) command or
global variable definition inside main() - you'll see the first line of main()
is checking assert_outside_chroot so I don't think you can this script as is. :)

On 2010/01/25 23:16:57, seano wrote:
> On 2010/01/25 23:04:29, ericli wrote:
> > I think you guys do not want to run it inside chroot? what is the deal here?
> 
> No, we have to now and for the forseeable future IMO.

http://codereview.chromium.org/546137/diff/1/13#newcode18
src/scripts/run_remote_tests.sh:18: set -e
I'm already running this in main() - the general pattern I've seen in other
scripts is to not set -e until after FLAGS "$@".

[ericli, 2010-01-25 23:29:59]

http://codereview.chromium.org/546137/diff/4001/4011
File src/scripts/remote_access.sh (left):

http://codereview.chromium.org/546137/diff/4001/4011#oldcode17
src/scripts/remote_access.sh:17: UserKnownHostsFile=$TMP_KNOWN_HOSTS
root@$FLAGS_remote "$@")
Agree. I didn't think about image_to_live.sh, which is outside autotest code.

[ericli, 2010-01-26 00:08:26]

http://codereview.chromium.org/546137/diff/4001/4013
File src/scripts/run_remote_tests.sh (right):

http://codereview.chromium.org/546137/diff/4001/4013#newcode121
src/scripts/run_remote_tests.sh:121: cp -r
"${SRC_ROOT}/third_party/autotest/files" "${TMP}/autotest"
why you want to copy it to /tmp?

There is a pre-compiled copy under chroot/usr/local/autotest.

[seano, 2010-01-27 09:06:38]

http://codereview.chromium.org/546137/diff/1/10
File src/scripts/mod_for_test_scripts/ssh_keys/testing_rsa (right):

http://codereview.chromium.org/546137/diff/1/10#newcode2
src/scripts/mod_for_test_scripts/ssh_keys/testing_rsa:2:
MIIEoAIBAAKCAQEAvsNpFdK5lb0GfKx+FgsrsM/2+aZVFYXHMPdvGtTz63ciRhq0
On 2010/01/25 23:27:20, kmixter1 wrote:
> Did you remove this key from its old location?  Maybe you did and I just can't
> tell from rietveld.

git mv, not sure how that plays with the rietveld UI.

http://codereview.chromium.org/546137/diff/1/11
File src/scripts/remote_access.sh (left):

http://codereview.chromium.org/546137/diff/1/11#oldcode31
src/scripts/remote_access.sh:31: chmod 0400 $TMP_PRIVATE_KEY
On 2010/01/25 23:27:20, kmixter1 wrote:
> What if a different --private_key is passed to the script - in that case
keeping
> around the setting of the mode to 0400 seems like the right thing to do.

Done.

http://codereview.chromium.org/546137/diff/1/13
File src/scripts/run_remote_tests.sh (right):

http://codereview.chromium.org/546137/diff/1/13#newcode16
src/scripts/run_remote_tests.sh:16: assert_inside_chroot
On 2010/01/25 23:27:20, kmixter1 wrote:
> I hope not.  I definitely don't run it inside the chroot now and prefer to
> always avoid requiring folks to run anything inside the chroot (would even
> prefer creating an outer script that enter_chroot's an inner script).
> 
> Also, for readability, please put anything that's not a source (.) command or
> global variable definition inside main() - you'll see the first line of main()
> is checking assert_outside_chroot so I don't think you can this script as is.
:)
> 
> On 2010/01/25 23:16:57, seano wrote:
> > On 2010/01/25 23:04:29, ericli wrote:
> > > I think you guys do not want to run it inside chroot? what is the deal
here?
> > 
> > No, we have to now and for the forseeable future IMO.
> 
> 

Understood and reverted.

http://codereview.chromium.org/546137/diff/1/13#newcode18
src/scripts/run_remote_tests.sh:18: set -e
On 2010/01/25 23:27:20, kmixter1 wrote:
> I'm already running this in main() - the general pattern I've seen in other
> scripts is to not set -e until after FLAGS "$@".

Done.

http://codereview.chromium.org/546137/diff/4001/4011
File src/scripts/remote_access.sh (left):

http://codereview.chromium.org/546137/diff/4001/4011#oldcode17
src/scripts/remote_access.sh:17: UserKnownHostsFile=$TMP_KNOWN_HOSTS
root@$FLAGS_remote "$@")
On 2010/01/25 23:30:00, ericli wrote:
> Agree. I didn't think about image_to_live.sh, which is outside autotest code.
> 

Re-added for now. Good catch.

http://codereview.chromium.org/546137/diff/4001/4013
File src/scripts/run_remote_tests.sh (right):

http://codereview.chromium.org/546137/diff/4001/4013#newcode121
src/scripts/run_remote_tests.sh:121: cp -r
"${SRC_ROOT}/third_party/autotest/files" "${TMP}/autotest"
On 2010/01/26 00:08:27, ericli wrote:
> why you want to copy it to /tmp?
> 
> There is a pre-compiled copy under chroot/usr/local/autotest.

Right. I now change this to check for the prebuilt image (in the autotest
install dir, currently in chroot but subject to change any time).

[petkov, 2010-01-27 15:46:26]

http://codereview.chromium.org/546137/diff/8001/9008
File src/scripts/build_autotest.sh (right):

http://codereview.chromium.org/546137/diff/8001/9008#newcode1
src/scripts/build_autotest.sh:1: #!/bin/bash
You've renamed an older version of make_autotest.sh -- please rename the latest
one.

http://codereview.chromium.org/546137/diff/8001/9013
File src/scripts/run_remote_tests.sh (right):

http://codereview.chromium.org/546137/diff/8001/9013#newcode179
src/scripts/run_remote_tests.sh:179: TODO call from correct install dir
This needs to be a comment? Also, TODO(seano)...

[petkov, 2010-01-27 17:12:05]

http://codereview.chromium.org/546137/diff/8001/9013
File src/scripts/run_remote_tests.sh (right):

http://codereview.chromium.org/546137/diff/8001/9013#newcode127
src/scripts/run_remote_tests.sh:127: local search_path=$(echo
${AUTOTEST_INSTALL_DIR}/{client,server}/{tests,site_tests})
80 chars

[seano, 2010-01-27 19:51:47]

http://codereview.chromium.org/546137/diff/8001/9008
File src/scripts/build_autotest.sh (right):

http://codereview.chromium.org/546137/diff/8001/9008#newcode1
src/scripts/build_autotest.sh:1: #!/bin/bash
On 2010/01/27 15:46:26, petkov wrote:
> You've renamed an older version of make_autotest.sh -- please rename the
latest
> one.
> 

oops. Re-synced now.

http://codereview.chromium.org/546137/diff/8001/9013
File src/scripts/run_remote_tests.sh (right):

http://codereview.chromium.org/546137/diff/8001/9013#newcode127
src/scripts/run_remote_tests.sh:127: local search_path=$(echo
${AUTOTEST_INSTALL_DIR}/{client,server}/{tests,site_tests})
On 2010/01/27 17:12:06, petkov wrote:
> 80 chars
> 

Done.

http://codereview.chromium.org/546137/diff/8001/9013#newcode179
src/scripts/run_remote_tests.sh:179: TODO call from correct install dir
On 2010/01/27 15:46:26, petkov wrote:
> This needs to be a comment? Also, TODO(seano)...

Done.