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
1=== modified file 'provider/local/environprovider.go'
2--- provider/local/environprovider.go 2014-02-12 02:56:13 +0000
3+++ provider/local/environprovider.go 2014-02-14 03:42:42 +0000
4@@ -51,8 +51,16 @@
5 // for backwards compatibility: older versions did not store the namespace
6 // in config.
7 if namespace, _ := cfg.UnknownAttrs()["namespace"].(string); namespace == "" {
8+ username := os.Getenv("USER")
9+ if username == "" {
10+ u, err := userCurrent()
11+ if err != nil {
12+ return nil, fmt.Errorf("failed to determine username for namespace: %v", err)
13+ }
14+ username = u.Username
15+ }
16 var err error
17- namespace = fmt.Sprintf("%s-%s", os.Getenv("USER"), cfg.Name())
18+ namespace = fmt.Sprintf("%s-%s", username, cfg.Name())
19 cfg, err = cfg.Apply(map[string]interface{}{"namespace": namespace})
20 if err != nil {
21 return nil, fmt.Errorf("failed to create namespace: %v", err)
22
23=== modified file 'provider/local/environprovider_test.go'
24--- provider/local/environprovider_test.go 2014-01-31 08:44:08 +0000
25+++ provider/local/environprovider_test.go 2014-02-14 03:42:42 +0000
26@@ -4,6 +4,9 @@
27 package local_test
28
29 import (
30+ "errors"
31+ "os/user"
32+
33 "github.com/loggo/loggo"
34 gc "launchpad.net/gocheck"
35
36@@ -53,10 +56,11 @@
37 s.PatchEnvironment("ftp_proxy", "")
38 s.PatchEnvironment("FTP_PROXY", "")
39 s.HookCommandOutput(&utils.AptCommandOutput, nil, nil)
40- restore := testbase.PatchValue(local.CheckLocalPort, func(port int, desc string) error {
41+ s.PatchValue(local.CheckLocalPort, func(port int, desc string) error {
42 return nil
43 })
44- s.AddSuiteCleanup(func(*gc.C) { restore() })
45+ restore := local.MockAddressForInterface()
46+ s.AddCleanup(func(*gc.C) { restore() })
47 }
48
49 func (s *prepareSuite) TestPrepareCapturesEnvironment(c *gc.C) {
50@@ -67,7 +71,6 @@
51 c.Assert(err, gc.IsNil)
52 provider, err := environs.Provider(provider.Local)
53 c.Assert(err, gc.IsNil)
54- defer local.MockAddressForInterface()()
55
56 for i, test := range []struct {
57 message string
58@@ -229,3 +232,50 @@
59 }
60 }
61 }
62+
63+func (s *prepareSuite) TestPrepareNamespace(c *gc.C) {
64+ s.PatchValue(local.DetectAptProxies, func() (osenv.ProxySettings, error) {
65+ return osenv.ProxySettings{}, nil
66+ })
67+ basecfg, err := config.New(config.UseDefaults, map[string]interface{}{
68+ "type": "local",
69+ "name": "test",
70+ })
71+ provider, err := environs.Provider("local")
72+ c.Assert(err, gc.IsNil)
73+
74+ type test struct {
75+ userEnv string
76+ userOS string
77+ userOSErr error
78+ namespace string
79+ err string
80+ }
81+ tests := []test{{
82+ userEnv: "someone",
83+ userOS: "other",
84+ namespace: "someone-test",
85+ }, {
86+ userOS: "other",
87+ namespace: "other-test",
88+ }, {
89+ userOSErr: errors.New("oh noes"),
90+ err: "failed to determine username for namespace: oh noes",
91+ }}
92+
93+ for i, test := range tests {
94+ c.Logf("test %d: %v", i, test)
95+ s.PatchEnvironment("USER", test.userEnv)
96+ s.PatchValue(local.UserCurrent, func() (*user.User, error) {
97+ return &user.User{Username: test.userOS}, test.userOSErr
98+ })
99+ env, err := provider.Prepare(basecfg)
100+ if test.err == "" {
101+ c.Assert(err, gc.IsNil)
102+ cfg := env.Config()
103+ c.Assert(cfg.UnknownAttrs()["namespace"], gc.Equals, test.namespace)
104+ } else {
105+ c.Assert(err, gc.ErrorMatches, test.err)
106+ }
107+ }
108+}
109
110=== modified file 'provider/local/export_test.go'
111--- provider/local/export_test.go 2014-02-12 02:56:13 +0000
112+++ provider/local/export_test.go 2014-02-14 03:42:42 +0000
113@@ -14,6 +14,7 @@
114 FinishBootstrap = &finishBootstrap
115 CheckLocalPort = &checkLocalPort
116 DetectAptProxies = &detectAptProxies
117+ UserCurrent = &userCurrent
118 )
119
120 // SetRootCheckFunction allows tests to override the check for a root user.

Subscribers

People subscribed via source and target branches

to status/vote changes: