Merge lp:~axwalk/juju-core/lp1274780-osuser-fallback into lp:~go-bot/juju-core/trunk

Proposed by Andrew Wilkins
Status: Merged
Approved by: Andrew Wilkins
Approved revision: no longer in the source branch.
Merged at revision: 2326
Proposed branch: lp:~axwalk/juju-core/lp1274780-osuser-fallback
Merge into: lp:~go-bot/juju-core/trunk
Prerequisite: lp:~axwalk/juju-core/local-provider-testability
Diff against target: 120 lines (+63/-4)
3 files modified
provider/local/environprovider.go (+9/-1)
provider/local/environprovider_test.go (+53/-3)
provider/local/export_test.go (+1/-0)
To merge this branch: bzr merge lp:~axwalk/juju-core/lp1274780-osuser-fallback
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+204173@code.launchpad.net

Commit message

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://codereview.appspot.com/58970043/

Description of the change

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://codereview.appspot.com/58970043/

To post a comment you must log in.
Revision history for this message
Andrew Wilkins (axwalk) wrote :
Download full text (4.0 KiB)

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 ...

Read more...

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

LGTM with one comment.

https://codereview.appspot.com/58970043/diff/1/provider/local/environprovider_test.go
File provider/local/environprovider_test.go (right):

https://codereview.appspot.com/58970043/diff/1/provider/local/environprovider_test.go#newcode262
provider/local/environprovider_test.go:262: userOSErr: errors.New("oh
noes"),
You could just define userOSErr as string and construct an error from it
in PatchValue below.

https://codereview.appspot.com/58970043/

Revision history for this message
Dave Cheney (dave-cheney) wrote :

Why not just always call http://golang.org/pkg/os/user/#Current, that has
the same semantics as you described.

On Fri, Jan 31, 2014 at 8:27 PM, Dimiter Naydenov <
<email address hidden>> wrote:

> LGTM with one comment.
>
>
>
> https://codereview.appspot.com/58970043/diff/1/provider/local/environprovider_test.go
> File provider/local/environprovider_test.go (right):
>
>
> https://codereview.appspot.com/58970043/diff/1/provider/local/environprovider_test.go#newcode262
> provider/local/environprovider_test.go:262: userOSErr: errors.New("oh
> noes"),
> You could just define userOSErr as string and construct an error from it
> in PatchValue below.
>
> https://codereview.appspot.com/58970043/
>
> --
>
> https://code.launchpad.net/~axwalk/juju-core/lp1274780-osuser-fallback/+merge/204173
> You are subscribed to branch lp:juju-core.
>

Revision history for this message
Dimiter Naydenov (dimitern) wrote :

Because of this issue https://code.google.com/p/go/issues/detail?id=6376
which I discovered some time ago and might still be a problem.

Revision history for this message
Andrew Wilkins (axwalk) wrote :

> Because of this issue https://code.google.com/p/go/issues/detail?id=6376
> which I discovered some time ago and might still be a problem.

Also, $USER can be overridden without var override hacks. Using an env var is more easily testable.

Revision history for this message
Andrew Wilkins (axwalk) wrote :

https://codereview.appspot.com/58970043/diff/1/provider/local/environprovider_test.go
File provider/local/environprovider_test.go (right):

https://codereview.appspot.com/58970043/diff/1/provider/local/environprovider_test.go#newcode262
provider/local/environprovider_test.go:262: userOSErr: errors.New("oh
noes"),
On 2014/01/31 09:25:07, dimitern wrote:
> You could just define userOSErr as string and construct an error from
it in
> PatchValue below.

I was going to initially, but it's more long-winded that way. You need
to check if the string != "", and only then create an error. I'll leave
it unless you strongly object.

https://codereview.appspot.com/58970043/

Revision history for this message
Tim Penhey (thumper) wrote :
Revision history for this message
Andrew Wilkins (axwalk) wrote :

https://codereview.appspot.com/58970043/diff/1/provider/local/environprovider.go
File provider/local/environprovider.go (right):

https://codereview.appspot.com/58970043/diff/1/provider/local/environprovider.go#newcode60
provider/local/environprovider.go:60: username = u.Username
On 2014/02/13 21:58:04, thumper wrote:
> should we lowercase this?

Nope, *nix usernames are case sensitive.

https://codereview.appspot.com/58970043/

Revision history for this message
Tim Penhey (thumper) wrote :

On 2014/02/14 01:03:27, axw wrote:

https://codereview.appspot.com/58970043/diff/1/provider/local/environprovider.go
> File provider/local/environprovider.go (right):

https://codereview.appspot.com/58970043/diff/1/provider/local/environprovider.go#newcode60
> provider/local/environprovider.go:60: username = u.Username
> On 2014/02/13 21:58:04, thumper wrote:
> > should we lowercase this?

> Nope, *nix usernames are case sensitive.

LGTM

https://codereview.appspot.com/58970043/

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (204.4 KiB)

The attempt to merge lp:~axwalk/juju-core/lp1274780-osuser-fallback into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.017s
ok launchpad.net/juju-core/agent 1.214s
ok launchpad.net/juju-core/agent/tools 0.264s
ok launchpad.net/juju-core/bzr 6.715s
ok launchpad.net/juju-core/cert 3.834s
ok launchpad.net/juju-core/charm 0.565s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.037s
ok launchpad.net/juju-core/cloudinit/sshinit 1.158s
ok launchpad.net/juju-core/cmd 0.290s
ok launchpad.net/juju-core/cmd/charm-admin 0.786s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]

----------------------------------------------------------------------
PANIC: bootstrap_test.go:62: BootstrapSuite.TearDownTest

... Panic: no reachable servers (PC=0x414321)

/usr/lib/go/src/pkg/runtime/panic.c:229
  in panic
/home/tarmac/trees/src/launchpad.net/juju-core/testing/mgo.go:254
  in MgoInstance.MustDial
/home/tarmac/trees/src/launchpad.net/juju-core/testing/mgo.go:310
  in MgoInstance.Reset
/home/tarmac/trees/src/launchpad.net/juju-core/testing/mgo.go:396
  in MgoSuite.TearDownTest
bootstrap_test.go:64
  in BootstrapSuite.TearDownTest

----------------------------------------------------------------------
PANIC: bootstrap_test.go:106: BootstrapSuite.TestAllowRetries

[LOG] 74.70739 DEBUG juju.environs.tools no architecture specified when finding tools, looking for any
[LOG] 74.70745 DEBUG juju.environs.tools no series specified when finding tools, looking for any
[LOG] 74.70761 DEBUG juju.environs.simplestreams fetchData failed for "file:///tmp/gocheck-2781055864473387780/0/tools/streams/v1/index.sjson": stat /tmp/gocheck-2781055864473387780/0/tools/streams/v1/index.sjson: no such file or directory
[LOG] 74.70768 DEBUG juju.environs.simplestreams cannot load index "file:///tmp/gocheck-2781055864473387780/0/tools/streams/v1/index.sjson": invalid URL "file:///tmp/gocheck-2781055864473387780/0/tools/streams/v1/index.sjson" not found
[LOG] 74.70780 DEBUG juju.environs.simplestreams fetchData failed for "file:///tmp/gocheck-2781055864473387780/0/tools/streams/v1/index.json": stat /tmp/gocheck-2781055864473387780/0/tools/streams/v1/index.json: no such file or directory
[LOG] 74.70786 DEBUG juju.environs.simplestreams cannot load index "file:///tmp/gocheck-2781055864473387780/0/tools/streams/v1/index.json": invalid URL "file:///tmp/gocheck-2781055864473387780/0/tools/streams/v1/index.json" not found
[LOG] 74.71044 INFO juju.environs.tools Writing tools/streams/v1/index.json
[LOG] 74.71108 INFO juju.environs.tools Writing tools/streams/v1/com.ubuntu.juju:released:tools.json
test 0: no tools uploaded, first check has no retries; no matching binary in source; sync fails with no second attempt

[LOG] 74.71198 INFO juju.provider.dummy reset environment
[LOG] 74.73037 INFO juju Reset successfully reset admin password
[LOG] 74.73184 DEBUG juju.environs.configstore Making /tmp/gocheck-2781055864473387780/1/.juju/environments
clearing private storag...

Revision history for this message
Go Bot (go-bot) wrote :
Download full text (10.3 KiB)

The attempt to merge lp:~axwalk/juju-core/lp1274780-osuser-fallback into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.016s
ok launchpad.net/juju-core/agent 0.752s
ok launchpad.net/juju-core/agent/tools 0.298s
ok launchpad.net/juju-core/bzr 6.846s
ok launchpad.net/juju-core/cert 3.893s
ok launchpad.net/juju-core/charm 0.577s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.038s
ok launchpad.net/juju-core/cloudinit/sshinit 1.190s
ok launchpad.net/juju-core/cmd 0.251s
ok launchpad.net/juju-core/cmd/charm-admin 0.821s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 235.014s
ok launchpad.net/juju-core/cmd/jujud 60.122s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 10.370s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/constraints 0.028s
ok launchpad.net/juju-core/container 0.048s
ok launchpad.net/juju-core/container/factory 0.051s
ok launchpad.net/juju-core/container/kvm 0.262s
ok launchpad.net/juju-core/container/kvm/mock 0.038s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.286s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.252s
ok launchpad.net/juju-core/environs 3.014s
ok launchpad.net/juju-core/environs/bootstrap 4.453s
ok launchpad.net/juju-core/environs/cloudinit 0.641s
ok launchpad.net/juju-core/environs/config 3.139s
ok launchpad.net/juju-core/environs/configstore 0.039s
ok launchpad.net/juju-core/environs/filestorage 0.032s
ok launchpad.net/juju-core/environs/httpstorage 0.918s
ok launchpad.net/juju-core/environs/imagemetadata 0.626s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.059s
ok launchpad.net/juju-core/environs/jujutest 0.283s
ok launchpad.net/juju-core/environs/manual 9.680s
ok launchpad.net/juju-core/environs/simplestreams 0.357s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 1.270s
ok launchpad.net/juju-core/environs/storage 1.207s
ok launchpad.net/juju-core/environs/sync 34.372s
ok launchpad.net/juju-core/environs/testing 0.290s
ok launchpad.net/juju-core/environs/tools 6.917s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.015s
ok launchpad.net/juju-core/instance 0.024s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 23.752s
ok launchpad.net/juju-core/juju/osenv 0.020s
? launchpad.net/juju-core/juju/testing [no test files]
ok launchpad.net/juju-core/log 0.015s
ok launchpad.net/juju-core/log/syslog 0.026s
? launchpad.net/juju-cor...

Revision history for this message
Go Bot (go-bot) wrote :

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'provider/local/environprovider.go'
--- provider/local/environprovider.go 2014-02-12 02:56:13 +0000
+++ provider/local/environprovider.go 2014-02-14 03:42:42 +0000
@@ -51,8 +51,16 @@
51 // for backwards compatibility: older versions did not store the namespace51 // for backwards compatibility: older versions did not store the namespace
52 // in config.52 // in config.
53 if namespace, _ := cfg.UnknownAttrs()["namespace"].(string); namespace == "" {53 if namespace, _ := cfg.UnknownAttrs()["namespace"].(string); namespace == "" {
54 username := os.Getenv("USER")
55 if username == "" {
56 u, err := userCurrent()
57 if err != nil {
58 return nil, fmt.Errorf("failed to determine username for namespace: %v", err)
59 }
60 username = u.Username
61 }
54 var err error62 var err error
55 namespace = fmt.Sprintf("%s-%s", os.Getenv("USER"), cfg.Name())63 namespace = fmt.Sprintf("%s-%s", username, cfg.Name())
56 cfg, err = cfg.Apply(map[string]interface{}{"namespace": namespace})64 cfg, err = cfg.Apply(map[string]interface{}{"namespace": namespace})
57 if err != nil {65 if err != nil {
58 return nil, fmt.Errorf("failed to create namespace: %v", err)66 return nil, fmt.Errorf("failed to create namespace: %v", err)
5967
=== 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-02-14 03:42:42 +0000
@@ -4,6 +4,9 @@
4package local_test4package local_test
55
6import (6import (
7 "errors"
8 "os/user"
9
7 "github.com/loggo/loggo"10 "github.com/loggo/loggo"
8 gc "launchpad.net/gocheck"11 gc "launchpad.net/gocheck"
912
@@ -53,10 +56,11 @@
53 s.PatchEnvironment("ftp_proxy", "")56 s.PatchEnvironment("ftp_proxy", "")
54 s.PatchEnvironment("FTP_PROXY", "")57 s.PatchEnvironment("FTP_PROXY", "")
55 s.HookCommandOutput(&utils.AptCommandOutput, nil, nil)58 s.HookCommandOutput(&utils.AptCommandOutput, nil, nil)
56 restore := testbase.PatchValue(local.CheckLocalPort, func(port int, desc string) error {59 s.PatchValue(local.CheckLocalPort, func(port int, desc string) error {
57 return nil60 return nil
58 })61 })
59 s.AddSuiteCleanup(func(*gc.C) { restore() })62 restore := local.MockAddressForInterface()
63 s.AddCleanup(func(*gc.C) { restore() })
60}64}
6165
62func (s *prepareSuite) TestPrepareCapturesEnvironment(c *gc.C) {66func (s *prepareSuite) TestPrepareCapturesEnvironment(c *gc.C) {
@@ -67,7 +71,6 @@
67 c.Assert(err, gc.IsNil)71 c.Assert(err, gc.IsNil)
68 provider, err := environs.Provider(provider.Local)72 provider, err := environs.Provider(provider.Local)
69 c.Assert(err, gc.IsNil)73 c.Assert(err, gc.IsNil)
70 defer local.MockAddressForInterface()()
7174
72 for i, test := range []struct {75 for i, test := range []struct {
73 message string76 message string
@@ -229,3 +232,50 @@
229 }232 }
230 }233 }
231}234}
235
236func (s *prepareSuite) TestPrepareNamespace(c *gc.C) {
237 s.PatchValue(local.DetectAptProxies, func() (osenv.ProxySettings, error) {
238 return osenv.ProxySettings{}, nil
239 })
240 basecfg, err := config.New(config.UseDefaults, map[string]interface{}{
241 "type": "local",
242 "name": "test",
243 })
244 provider, err := environs.Provider("local")
245 c.Assert(err, gc.IsNil)
246
247 type test struct {
248 userEnv string
249 userOS string
250 userOSErr error
251 namespace string
252 err string
253 }
254 tests := []test{{
255 userEnv: "someone",
256 userOS: "other",
257 namespace: "someone-test",
258 }, {
259 userOS: "other",
260 namespace: "other-test",
261 }, {
262 userOSErr: errors.New("oh noes"),
263 err: "failed to determine username for namespace: oh noes",
264 }}
265
266 for i, test := range tests {
267 c.Logf("test %d: %v", i, test)
268 s.PatchEnvironment("USER", test.userEnv)
269 s.PatchValue(local.UserCurrent, func() (*user.User, error) {
270 return &user.User{Username: test.userOS}, test.userOSErr
271 })
272 env, err := provider.Prepare(basecfg)
273 if test.err == "" {
274 c.Assert(err, gc.IsNil)
275 cfg := env.Config()
276 c.Assert(cfg.UnknownAttrs()["namespace"], gc.Equals, test.namespace)
277 } else {
278 c.Assert(err, gc.ErrorMatches, test.err)
279 }
280 }
281}
232282
=== modified file 'provider/local/export_test.go'
--- provider/local/export_test.go 2014-02-12 02:56:13 +0000
+++ provider/local/export_test.go 2014-02-14 03:42:42 +0000
@@ -14,6 +14,7 @@
14 FinishBootstrap = &finishBootstrap14 FinishBootstrap = &finishBootstrap
15 CheckLocalPort = &checkLocalPort15 CheckLocalPort = &checkLocalPort
16 DetectAptProxies = &detectAptProxies16 DetectAptProxies = &detectAptProxies
17 UserCurrent = &userCurrent
17)18)
1819
19// SetRootCheckFunction allows tests to override the check for a root user.20// SetRootCheckFunction allows tests to override the check for a root user.

Subscribers

People subscribed via source and target branches

to status/vote changes: