Code review comment for lp:~frankban/juju-core/bug-1166224-service-deploy-panic

Reviewers: mp+157836_code.launchpad.net,

Message:
Please take a look.

Description:
Fix ServiceDeploy panic when called via the API.

Fix the panic that happens when calling ServiceDeploy via the juju-core
API.

Add a global charm.CacheDir variable, in order to avoid the call to
config.JujuHomePath inside the CharmStore.Get method.

charm.CacheDir is originally empty, and is then set by the
machineAgent.Run
method.

Pre-implementation call with Roger.

https://code.launchpad.net/~frankban/juju-core/bug-1166224-service-deploy-panic/+merge/157836

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M charm/repo.go
   M charm/repo_test.go
   M cmd/jujud/machine.go
   M cmd/jujud/machine_test.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: <email address hidden>
+New revision:
<email address hidden>

Index: charm/repo.go
=== modified file 'charm/repo.go'
--- charm/repo.go 2013-04-03 23:29:07 +0000
+++ charm/repo.go 2013-04-08 16:36:35 +0000
@@ -7,7 +7,6 @@
   "fmt"
   "io"
   "io/ioutil"
- "launchpad.net/juju-core/environs/config"
   "launchpad.net/juju-core/log"
   "net/http"
   "net/url"
@@ -16,6 +15,9 @@
   "strings"
  )

+// CacheDir stores the charm cache directory path.
+var CacheDir string
+
  // InfoResponse is sent by the charm store in response to charm-info
requests.
  type InfoResponse struct {
   Revision int `json:"revision"` // Zero is valid. Can't omitempty.
@@ -211,8 +213,11 @@

  // Get returns the charm referenced by curl.
  func (s *CharmStore) Get(curl *URL) (Charm, error) {
- cachePath := config.JujuHomePath("cache")
- if err := os.MkdirAll(cachePath, 0755); err != nil {
+ // MachineAgent.Run should have already set the CacheDir.
+ if CacheDir == "" {
+ panic("the charm cache directory path is empty")
+ }
+ if err := os.MkdirAll(CacheDir, 0755); err != nil {
    return nil, err
   }
   rev, digest, err := s.revision(curl)
@@ -224,14 +229,14 @@
   } else if curl.Revision != rev {
    return nil, fmt.Errorf("charm: store returned charm with wrong revision
for %q", curl.String())
   }
- path := filepath.Join(cachePath, Quote(curl.String())+".charm")
+ path := filepath.Join(CacheDir, Quote(curl.String())+".charm")
   if verify(path, digest) != nil {
    resp, err := http.Get(s.baseURL + "/charm/" +
url.QueryEscape(curl.Path()))
    if err != nil {
     return nil, err
    }
    defer resp.Body.Close()
- f, err := ioutil.TempFile(cachePath, "charm-download")
+ f, err := ioutil.TempFile(CacheDir, "charm-download")
    if err != nil {
     return nil, err
    }

Index: charm/repo_test.go
=== modified file 'charm/repo_test.go'
--- charm/repo_test.go 2013-04-03 23:29:07 +0000
+++ charm/repo_test.go 2013-04-09 09:51:19 +0000
@@ -8,7 +8,6 @@
   "io/ioutil"
   . "launchpad.net/gocheck"
   "launchpad.net/juju-core/charm"
- "launchpad.net/juju-core/environs/config"
   "launchpad.net/juju-core/log"
   "launchpad.net/juju-core/testing"
   "net"
@@ -150,23 +149,24 @@
  type StoreSuite struct {
   server *MockStore
   store *charm.CharmStore
- oldJujuHome string
+ oldCacheDir string
  }

  var _ = Suite(&StoreSuite{})

  func (s *StoreSuite) SetUpSuite(c *C) {
   s.server = NewMockStore(c)
+ s.oldCacheDir = charm.CacheDir
  }

  func (s *StoreSuite) SetUpTest(c *C) {
- s.oldJujuHome = config.SetJujuHome(c.MkDir())
+ charm.CacheDir = c.MkDir()
   s.store = charm.NewStore("http://127.0.0.1:4444")
   s.server.downloads = nil
  }

  func (s *StoreSuite) TearDownSuite(c *C) {
- config.SetJujuHome(s.oldJujuHome)
+ charm.CacheDir = s.oldCacheDir
   s.server.lis.Close()
  }

@@ -245,12 +245,12 @@
  }

  func (s *StoreSuite) TestGetBadCache(c *C) {
- c.Assert(os.Mkdir(config.JujuHomePath("cache"), 0777), IsNil)
+ c.Assert(os.Mkdir(filepath.Join(charm.CacheDir, "cache"), 0777), IsNil)
   base := "cs:series/good"
   curl := charm.MustParseURL(base)
   revCurl := charm.MustParseURL(base + "-23")
   name := charm.Quote(revCurl.String()) + ".charm"
- err := ioutil.WriteFile(config.JujuHomePath("cache", name), nil, 0666)
+ err := ioutil.WriteFile(filepath.Join(charm.CacheDir, "cache", name),
nil, 0666)
   c.Assert(err, IsNil)
   ch, err := s.store.Get(curl)
   c.Assert(err, IsNil)

Index: cmd/jujud/machine.go
=== modified file 'cmd/jujud/machine.go'
--- cmd/jujud/machine.go 2013-04-05 13:47:17 +0000
+++ cmd/jujud/machine.go 2013-04-08 16:36:35 +0000
@@ -3,6 +3,7 @@
  import (
   "fmt"
   "launchpad.net/gnuflag"
+ "launchpad.net/juju-core/charm"
   "launchpad.net/juju-core/cmd"
   "launchpad.net/juju-core/environs/agent"
   "launchpad.net/juju-core/log"
@@ -13,6 +14,7 @@
   "launchpad.net/juju-core/worker/machiner"
   "launchpad.net/juju-core/worker/provisioner"
   "launchpad.net/tomb"
+ "path/filepath"
   "time"
  )

@@ -58,6 +60,7 @@
   if err := a.Conf.read(state.MachineTag(a.MachineId)); err != nil {
    return err
   }
+ charm.CacheDir = filepath.Join(a.Conf.DataDir, "charmcache")
   defer log.Noticef("cmd/jujud: machine agent exiting")
   defer a.tomb.Done()

Index: cmd/jujud/machine_test.go
=== modified file 'cmd/jujud/machine_test.go'
--- cmd/jujud/machine_test.go 2013-04-03 15:33:42 +0000
+++ cmd/jujud/machine_test.go 2013-04-09 09:51:19 +0000
@@ -3,6 +3,7 @@
  import (
   "fmt"
   . "launchpad.net/gocheck"
+ "launchpad.net/juju-core/charm"
   "launchpad.net/juju-core/cmd"
   "launchpad.net/juju-core/environs/agent"
   "launchpad.net/juju-core/environs/dummy"
@@ -10,16 +11,26 @@
   "launchpad.net/juju-core/state/api"
   "launchpad.net/juju-core/state/watcher"
   "launchpad.net/juju-core/testing"
+ "path/filepath"
   "reflect"
   "time"
  )

  type MachineSuite struct {
   agentSuite
+ oldCacheDir string
  }

  var _ = Suite(&MachineSuite{})

+func (s *MachineSuite) SetUpSuite(c *C) {
+ s.oldCacheDir = charm.CacheDir
+}
+
+func (s *MachineSuite) TearDownSuite(c *C) {
+ charm.CacheDir = s.oldCacheDir
+}
+
  // primeAgent adds a new Machine to run the given jobs, and sets up the
  // machine agent's directory. It returns the new machine, the
  // agent's configuration and the tools currently running.
@@ -72,7 +83,7 @@
  }

  func (s *MachineSuite) TestRunStop(c *C) {
- m, _, _ := s.primeAgent(c, state.JobHostUnits)
+ m, ac, _ := s.primeAgent(c, state.JobHostUnits)
   a := s.newAgent(c, m)
   done := make(chan error)
   go func() {
@@ -81,6 +92,7 @@
   err := a.Stop()
   c.Assert(err, IsNil)
   c.Assert(<-done, IsNil)
+ c.Assert(charm.CacheDir, Equals, filepath.Join(ac.DataDir, "charmcache"))
  }

  func (s *MachineSuite) TestWithDeadMachine(c *C) {