Merge lp:~nuclearbob/utah/bug1155615 into lp:utah

[Max Brustkern]

This branch sets rc.local to use the file specified in -o if one exists.

[Andy Doan (doanac)]

On 04/26/2013 01:01 PM, Max Brustkern wrote:

> https://code.launchpad.net/~nuclearbob/utah/bug1155615/+merge/161205

> === modified file 'utah/client/runner.py'

> def __init__(self, install_type, runlist=None, result_class=Result,
> testdir=UTAH_DIR, state_agent=None,
> - resume=False, old_results=None):
> + resume=False, old_results=None, output=None):
>
> # Runlist URL passed through the command line
> self.testdir = testdir
> self.revision = "Unknown"
> + self.output = (output or
> + os.path.join('/', 'var', 'lib', 'utah', 'utah.out'))

does os.path.join really gain anything here? ie wouldn't it be more
concise to just do "/var/lib/utah/utah.out"? might even default it in
the parameters to the function so you don't have to check for None here.

> with open(rc_local, 'w') as fp:
> - fp.write(self.rc_local_content.format(runlist=runlist))
> + fp.write(self.rc_local_content.format(runlist=runlist,
> + output=self.output))

I think you forgot to update rc_local_content at the top of the file to
take in this new parameter. ie:

rc_local_content = """#!/bin/sh

/usr/bin/utah --resume -o {outout} -r {runlist}
"""

Also - would it be possible to add a test case to ensure we do write to
non-standard output directory?

[Max Brustkern (nuclearbob)]

I did forget to add that. Whoops. I can add a test case as well.

The reason I'm not doing the parameter default (although I suppose I could) is the old reason we didn't use parameter defaults in Machine: args is going to overwrite them with None if nothing gets passed.

As far as os.path.join is concerned, I don't think we actually need it in the vast majority of cases we're currently using it, but it always seems like a good idea? If we want to use that less, I'd like to be clearer on when we do and don't want to use it.

[lp:~nuclearbob/utah/bug1155615]

879. By Max Brustkern on 2013-04-26

Actually set output in rc_local_content

[Andy Doan (doanac)]

On 04/26/2013 02:18 PM, Max Brustkern wrote:
> As far as os.path.join is concerned, I don't think we actually need it in the vast majority of cases we're currently using it, but it always seems like a good idea? If we want to use that less, I'd like to be clearer on when we do and don't want to use it.

yeah - this isn't the only place. I guess I see it as useful when you
are joining variables like "basedir" and "fname". ie
  os.path.join(basedir, fname)

but maybe that's beyond the scope of this.

[Javier Collado (javier.collado)]

There's a problem in `client.py` because `args.output` is passed as a positional argument after a keyword argument and that's disallowed.

I already talked to Max about this and he's working on it.

review: Needs Fixing

[lp:~nuclearbob/utah/bug1155615]

880. By Max Brustkern on 2013-04-30

Specified keyword argument for output

[Javier Collado (javier.collado)]

Looks good to me.

Could you add a test case to verify that the contents of the `rc.local` file is correct? There's already a test to verify that the file is written, so it shouldn't be hard to verify its contents.

[lp:~nuclearbob/utah/bug1155615]

881. By Max Brustkern on 2013-05-01

Added testcase for contents of rc.local

[Max Brustkern (nuclearbob)]

Let me know how that looks.

review: Needs Resubmitting

[Javier Collado (javier.collado)]

Looks good. Thanks for the update.

review: Approve

=== modified file 'client.py'
--- client.py	2013-04-19 19:04:15 +0000
+++ client.py	2013-05-01 16:53:26 +0000
@@ -159,7 +159,7 @@
         runner = Runner(install_type=install_type, state_agent=state_agent,
                         result_class=result_class, runlist=runlist,
                         resume=resume, old_results=old_results,
-                        testdir=testdir)
+                        testdir=testdir, output=args.output)
         if resume:
             runner.load_state()
 

=== modified file 'debian/changelog'
--- debian/changelog	2013-04-26 15:08:17 +0000
+++ debian/changelog	2013-05-01 16:53:26 +0000
@@ -14,8 +14,9 @@
   * Make rsyslog timeout error message clearer (LP: #1169846)
   * Write to log UTAH version as soon as possible (LP: #1152216)
   * Fixed help output to use just one runlist (LP: #1112597)
+  * Setup rc.local to use output passed via command line (LP: #1155615)
 
- -- Max Brustkern <[e-mail]>  Wed, 24 Apr 2013 17:04:27 -0400
+ -- Max Brustkern <[e-mail]>  Fri, 26 Apr 2013 13:58:25 -0400
 
 utah (0.10ubuntu1) UNRELEASED; urgency=low
 

=== modified file 'utah/client/runner.py'
--- utah/client/runner.py	2013-04-26 13:40:06 +0000
+++ utah/client/runner.py	2013-05-01 16:53:26 +0000
@@ -58,7 +58,7 @@
 # run.
 rc_local_content = """#!/bin/sh
 
-/usr/bin/utah --resume -o /var/lib/utah/utah.out -r {runlist}
+/usr/bin/utah --resume -o {output} -r {runlist}
 """
 
 
@@ -140,11 +140,13 @@
 
     def __init__(self, install_type, runlist=None, result_class=Result,
                  testdir=UTAH_DIR, state_agent=None,
-                 resume=False, old_results=None):
+                 resume=False, old_results=None, output=None):
 
         # Runlist URL passed through the command line
         self.testdir = testdir
         self.revision = "Unknown"
+        self.output = (output or
+                       os.path.join('/', 'var', 'lib', 'utah', 'utah.out'))
 
         self.testsuitedir = os.path.join(testdir, 'testsuites')
 
@@ -239,7 +241,8 @@
 
         try:
             with open(rc_local, 'w') as fp:
-                fp.write(self.rc_local_content.format(runlist=runlist))
+                fp.write(self.rc_local_content.format(runlist=runlist,
+                         output=self.output))
                 os.fchmod(fp.fileno(),
                           stat.S_IRWXU | stat.S_IROTH | stat.S_IRGRP)
         except (IOError, OSError) as err:

=== modified file 'utah/client/tests/test_runner.py'
--- utah/client/tests/test_runner.py	2013-04-29 18:55:25 +0000
+++ utah/client/tests/test_runner.py	2013-05-01 16:53:26 +0000
@@ -49,9 +49,11 @@
         self.result_class = ResultYAML
         self.state_file = '/tmp/state.yaml'
         self.state_agent = StateAgentYAML(state_file=self.state_file)
+        self.output_file='/tmp/utah.out'
         self.runner = Runner(install_type='desktop',
                              result_class=self.result_class,
-                             state_agent=self.state_agent)
+                             state_agent=self.state_agent,
+                             output=self.output_file)
         self.bad_dir = "/tmp/does_not_exist"
 
         self.assertFalse(os.path.exists(self.bad_dir),
@@ -98,6 +100,15 @@
 
         self.assertTrue(os.path.exists(tmp_rc_local))
 
+    def test_rc_local_contents(self):
+        """Test rc.local output setting."""
+        tmp_rc_local = '/tmp/rc.local'
+        self.runner.setup_rc_local(tmp_rc_local)
+
+        with open(tmp_rc_local) as rc_local:
+            self.assertIn(self.output_file, rc_local.read(),
+                          'Output directory not written to rc.local')
+
     def test_run(self):
         """Test running all test suites."""
         retcode = self.runner.run()