Code review comment for lp:~thumper/juju-core/upgrade-fix-juju-run

Reviewers: mp+210951_code.launchpad.net,

Message:
Please take a look.

Description:
Fix the permissions on the tools dir on upgrade

The tempdir was created with 0700, and then renamed
to be the tools dir. Now we chmod to 0755 prior to
rename.

https://code.launchpad.net/~thumper/juju-core/upgrade-fix-juju-run/+merge/210951

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/75680043/

Affected files (+15, -0 lines):
   A [revision details]
   M agent/tools/diskmanager_test.go
   M agent/tools/toolsdir.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140313162936-3e8iq4csv4axq6z8
+New revision: <email address hidden>

Index: agent/tools/diskmanager_test.go
=== modified file 'agent/tools/diskmanager_test.go'
--- agent/tools/diskmanager_test.go 2013-09-25 00:18:35 +0000
+++ agent/tools/diskmanager_test.go 2014-03-14 01:15:39 +0000
@@ -6,6 +6,7 @@
  import (
   "bytes"
   "encoding/json"
+ "os"
   "path/filepath"

   gc "launchpad.net/gocheck"
@@ -101,4 +102,9 @@
   gotTools, err := s.manager.ReadTools(t.Version)
   c.Assert(err, gc.IsNil)
   c.Assert(*gotTools, gc.Equals, *t)
+ // Make sure that the tools directory is readable by the ubuntu user (for
+ // juju-run)
+ info, err := os.Stat(dir)
+ c.Assert(err, gc.IsNil)
+ c.Assert(info.Mode().Perm(), gc.Equals, os.FileMode(0755))
  }

Index: agent/tools/toolsdir.go
=== modified file 'agent/tools/toolsdir.go'
--- agent/tools/toolsdir.go 2014-02-03 14:31:54 +0000
+++ agent/tools/toolsdir.go 2014-03-14 01:16:20 +0000
@@ -113,6 +113,13 @@
    return err
   }

+ // The tempdir is created with 0700, so we need to make it more
+ // accessable for juju-run.
+ err = os.Chmod(dir, 0755)
+ if err != nil {
+ return err
+ }
+
   err = os.Rename(dir, SharedToolsDir(dataDir, tools.Version))
   // If we've failed to rename the directory, it may be because
   // the directory already exists - if ReadTools succeeds, we