Code review comment for lp:~axwalk/juju-core/lp1274780-osuser-fallback

Reviewers: mp+204173_code.launchpad.net,

Message:
Please take a look.

Description:
provider/local: fall back to os/user if $USER=""

If $USER is unset/empty, then fall back to os/user.
If that fails, then return an error.

Fixes lp:1274780

https://code.launchpad.net/~axwalk/juju-core/lp1274780-osuser-fallback/+merge/204173

Requires:
https://code.launchpad.net/~axwalk/juju-core/local-provider-testability/+merge/204169

(do not edit description out of merge proposal)

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

Affected files (+62, -1 lines):
   A [revision details]
   M provider/local/environprovider.go
   M provider/local/environprovider_test.go
   M provider/local/export_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: provider/local/environprovider.go
=== modified file 'provider/local/environprovider.go'
--- provider/local/environprovider.go 2014-01-31 08:44:08 +0000
+++ provider/local/environprovider.go 2014-01-31 09:04:29 +0000
@@ -51,8 +51,16 @@
   // for backwards compatibility: older versions did not store the namespace
   // in config.
   if namespace, _ := cfg.UnknownAttrs()["namespace"].(string); namespace
== "" {
+ username := os.Getenv("USER")
+ if username == "" {
+ u, err := userCurrent()
+ if err != nil {
+ return nil, fmt.Errorf("failed to determine username for
namespace: %v", err)
+ }
+ username = u.Username
+ }
    var err error
- namespace = fmt.Sprintf("%s-%s", os.Getenv("USER"), cfg.Name())
+ namespace = fmt.Sprintf("%s-%s", username, cfg.Name())
    cfg, err = cfg.Apply(map[string]interface{}{"namespace": namespace})
    if err != nil {
     return nil, fmt.Errorf("failed to create namespace: %v", err)

Index: provider/local/environprovider_test.go
=== modified file 'provider/local/environprovider_test.go'
--- provider/local/environprovider_test.go 2014-01-31 08:44:08 +0000
+++ provider/local/environprovider_test.go 2014-01-31 09:04:29 +0000
@@ -4,6 +4,9 @@
  package local_test

  import (
+ "errors"
+ "os/user"
+
   "github.com/loggo/loggo"
   gc "launchpad.net/gocheck"

@@ -229,3 +232,50 @@
    }
   }
  }
+
+func (s *prepareSuite) TestPrepareNamespace(c *gc.C) {
+ s.PatchValue(local.DetectAptProxies, func() (osenv.ProxySettings, error) {
+ return osenv.ProxySettings{}, nil
+ })
+ basecfg, err := config.New(config.UseDefaults, map[string]interface{}{
+ "type": "local",
+ "name": "test",
+ })
+ provider, err := environs.Provider("local")
+ c.Assert(err, gc.IsNil)
+
+ type test struct {
+ userEnv string
+ userOS string
+ userOSErr error
+ namespace string
+ err string
+ }
+ tests := []test{{
+ userEnv: "someone",
+ userOS: "other",
+ namespace: "someone-test",
+ }, {
+ userOS: "other",
+ namespace: "other-test",
+ }, {
+ userOSErr: errors.New("oh noes"),
+ err: "failed to determine username for namespace: oh noes",
+ }}
+
+ for i, test := range tests {
+ c.Logf("test %d: %v", i, test)
+ s.PatchEnvironment("USER", test.userEnv)
+ s.PatchValue(local.UserCurrent, func() (*user.User, error) {
+ return &user.User{Username: test.userOS}, test.userOSErr
+ })
+ env, err := provider.Prepare(basecfg)
+ if test.err == "" {
+ c.Assert(err, gc.IsNil)
+ cfg := env.Config()
+ c.Assert(cfg.UnknownAttrs()["namespace"], gc.Equals, test.namespace)
+ } else {
+ c.Assert(err, gc.ErrorMatches, test.err)
+ }
+ }
+}

Index: provider/local/export_test.go
=== modified file 'provider/local/export_test.go'
--- provider/local/export_test.go 2014-01-31 08:44:08 +0000
+++ provider/local/export_test.go 2014-01-31 09:04:29 +0000
@@ -13,6 +13,7 @@
   FinishBootstrap = &finishBootstrap
   CheckLocalPort = &checkLocalPort
   DetectAptProxies = &detectAptProxies
+ UserCurrent = &userCurrent
  )

  // SetRootCheckFunction allows tests to override the check for a root user.