Merge lp:~axwalk/juju-core/lp1274780-osuser-fallback into lp:~go-bot/juju-core/trunk
- lp1274780-osuser-fallback
- Merge into trunk
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 |
Related bugs: |
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
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
Andrew Wilkins (axwalk) wrote : | # |
Dimiter Naydenov (dimitern) wrote : | # |
LGTM with one comment.
https:/
File provider/
https:/
provider/
noes"),
You could just define userOSErr as string and construct an error from it
in PatchValue below.
Dave Cheney (dave-cheney) wrote : | # |
Why not just always call http://
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:/
> File provider/
>
>
> https:/
> provider/
> noes"),
> You could just define userOSErr as string and construct an error from it
> in PatchValue below.
>
> https:/
>
> --
>
> https:/
> You are subscribed to branch lp:juju-core.
>
Dimiter Naydenov (dimitern) wrote : | # |
Because of this issue https:/
which I discovered some time ago and might still be a problem.
Andrew Wilkins (axwalk) wrote : | # |
> Because of this issue https:/
> 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.
Andrew Wilkins (axwalk) wrote : | # |
https:/
File provider/
https:/
provider/
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.
Tim Penhey (thumper) wrote : | # |
https:/
File provider/
https:/
provider/
should we lowercase this?
Andrew Wilkins (axwalk) wrote : | # |
https:/
File provider/
https:/
provider/
On 2014/02/13 21:58:04, thumper wrote:
> should we lowercase this?
Nope, *nix usernames are case sensitive.
Tim Penhey (thumper) wrote : | # |
On 2014/02/14 01:03:27, axw wrote:
https:/
> File provider/
https:/
> provider/
> On 2014/02/13 21:58:04, thumper wrote:
> > should we lowercase this?
> Nope, *nix usernames are case sensitive.
LGTM
Go Bot (go-bot) wrote : | # |
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.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
-------
PANIC: bootstrap_
... Panic: no reachable servers (PC=0x414321)
/usr/lib/
in panic
/home/tarmac/
in MgoInstance.
/home/tarmac/
in MgoInstance.Reset
/home/tarmac/
in MgoSuite.
bootstrap_
in BootstrapSuite.
-------
PANIC: bootstrap_
[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.
[LOG] 74.70768 DEBUG juju.environs.
[LOG] 74.70780 DEBUG juju.environs.
[LOG] 74.70786 DEBUG juju.environs.
[LOG] 74.71044 INFO juju.environs.tools Writing tools/streams/
[LOG] 74.71108 INFO juju.environs.tools Writing tools/streams/
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.
clearing private storag...
Go Bot (go-bot) wrote : | # |
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.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
? launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
? launchpad.
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
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 | 51 | // for backwards compatibility: older versions did not store the namespace | 51 | // for backwards compatibility: older versions did not store the namespace |
6 | 52 | // in config. | 52 | // in config. |
7 | 53 | if namespace, _ := cfg.UnknownAttrs()["namespace"].(string); namespace == "" { | 53 | if namespace, _ := cfg.UnknownAttrs()["namespace"].(string); namespace == "" { |
8 | 54 | username := os.Getenv("USER") | ||
9 | 55 | if username == "" { | ||
10 | 56 | u, err := userCurrent() | ||
11 | 57 | if err != nil { | ||
12 | 58 | return nil, fmt.Errorf("failed to determine username for namespace: %v", err) | ||
13 | 59 | } | ||
14 | 60 | username = u.Username | ||
15 | 61 | } | ||
16 | 54 | var err error | 62 | var err error |
18 | 55 | namespace = fmt.Sprintf("%s-%s", os.Getenv("USER"), cfg.Name()) | 63 | namespace = fmt.Sprintf("%s-%s", username, cfg.Name()) |
19 | 56 | cfg, err = cfg.Apply(map[string]interface{}{"namespace": namespace}) | 64 | cfg, err = cfg.Apply(map[string]interface{}{"namespace": namespace}) |
20 | 57 | if err != nil { | 65 | if err != nil { |
21 | 58 | return nil, fmt.Errorf("failed to create namespace: %v", err) | 66 | return nil, fmt.Errorf("failed to create namespace: %v", err) |
22 | 59 | 67 | ||
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 | 4 | package local_test | 4 | package local_test |
28 | 5 | 5 | ||
29 | 6 | import ( | 6 | import ( |
30 | 7 | "errors" | ||
31 | 8 | "os/user" | ||
32 | 9 | |||
33 | 7 | "github.com/loggo/loggo" | 10 | "github.com/loggo/loggo" |
34 | 8 | gc "launchpad.net/gocheck" | 11 | gc "launchpad.net/gocheck" |
35 | 9 | 12 | ||
36 | @@ -53,10 +56,11 @@ | |||
37 | 53 | s.PatchEnvironment("ftp_proxy", "") | 56 | s.PatchEnvironment("ftp_proxy", "") |
38 | 54 | s.PatchEnvironment("FTP_PROXY", "") | 57 | s.PatchEnvironment("FTP_PROXY", "") |
39 | 55 | s.HookCommandOutput(&utils.AptCommandOutput, nil, nil) | 58 | s.HookCommandOutput(&utils.AptCommandOutput, nil, nil) |
41 | 56 | restore := testbase.PatchValue(local.CheckLocalPort, func(port int, desc string) error { | 59 | s.PatchValue(local.CheckLocalPort, func(port int, desc string) error { |
42 | 57 | return nil | 60 | return nil |
43 | 58 | }) | 61 | }) |
45 | 59 | s.AddSuiteCleanup(func(*gc.C) { restore() }) | 62 | restore := local.MockAddressForInterface() |
46 | 63 | s.AddCleanup(func(*gc.C) { restore() }) | ||
47 | 60 | } | 64 | } |
48 | 61 | 65 | ||
49 | 62 | func (s *prepareSuite) TestPrepareCapturesEnvironment(c *gc.C) { | 66 | func (s *prepareSuite) TestPrepareCapturesEnvironment(c *gc.C) { |
50 | @@ -67,7 +71,6 @@ | |||
51 | 67 | c.Assert(err, gc.IsNil) | 71 | c.Assert(err, gc.IsNil) |
52 | 68 | provider, err := environs.Provider(provider.Local) | 72 | provider, err := environs.Provider(provider.Local) |
53 | 69 | c.Assert(err, gc.IsNil) | 73 | c.Assert(err, gc.IsNil) |
54 | 70 | defer local.MockAddressForInterface()() | ||
55 | 71 | 74 | ||
56 | 72 | for i, test := range []struct { | 75 | for i, test := range []struct { |
57 | 73 | message string | 76 | message string |
58 | @@ -229,3 +232,50 @@ | |||
59 | 229 | } | 232 | } |
60 | 230 | } | 233 | } |
61 | 231 | } | 234 | } |
62 | 235 | |||
63 | 236 | func (s *prepareSuite) TestPrepareNamespace(c *gc.C) { | ||
64 | 237 | s.PatchValue(local.DetectAptProxies, func() (osenv.ProxySettings, error) { | ||
65 | 238 | return osenv.ProxySettings{}, nil | ||
66 | 239 | }) | ||
67 | 240 | basecfg, err := config.New(config.UseDefaults, map[string]interface{}{ | ||
68 | 241 | "type": "local", | ||
69 | 242 | "name": "test", | ||
70 | 243 | }) | ||
71 | 244 | provider, err := environs.Provider("local") | ||
72 | 245 | c.Assert(err, gc.IsNil) | ||
73 | 246 | |||
74 | 247 | type test struct { | ||
75 | 248 | userEnv string | ||
76 | 249 | userOS string | ||
77 | 250 | userOSErr error | ||
78 | 251 | namespace string | ||
79 | 252 | err string | ||
80 | 253 | } | ||
81 | 254 | tests := []test{{ | ||
82 | 255 | userEnv: "someone", | ||
83 | 256 | userOS: "other", | ||
84 | 257 | namespace: "someone-test", | ||
85 | 258 | }, { | ||
86 | 259 | userOS: "other", | ||
87 | 260 | namespace: "other-test", | ||
88 | 261 | }, { | ||
89 | 262 | userOSErr: errors.New("oh noes"), | ||
90 | 263 | err: "failed to determine username for namespace: oh noes", | ||
91 | 264 | }} | ||
92 | 265 | |||
93 | 266 | for i, test := range tests { | ||
94 | 267 | c.Logf("test %d: %v", i, test) | ||
95 | 268 | s.PatchEnvironment("USER", test.userEnv) | ||
96 | 269 | s.PatchValue(local.UserCurrent, func() (*user.User, error) { | ||
97 | 270 | return &user.User{Username: test.userOS}, test.userOSErr | ||
98 | 271 | }) | ||
99 | 272 | env, err := provider.Prepare(basecfg) | ||
100 | 273 | if test.err == "" { | ||
101 | 274 | c.Assert(err, gc.IsNil) | ||
102 | 275 | cfg := env.Config() | ||
103 | 276 | c.Assert(cfg.UnknownAttrs()["namespace"], gc.Equals, test.namespace) | ||
104 | 277 | } else { | ||
105 | 278 | c.Assert(err, gc.ErrorMatches, test.err) | ||
106 | 279 | } | ||
107 | 280 | } | ||
108 | 281 | } | ||
109 | 232 | 282 | ||
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 | 14 | FinishBootstrap = &finishBootstrap | 14 | FinishBootstrap = &finishBootstrap |
115 | 15 | CheckLocalPort = &checkLocalPort | 15 | CheckLocalPort = &checkLocalPort |
116 | 16 | DetectAptProxies = &detectAptProxies | 16 | DetectAptProxies = &detectAptProxies |
117 | 17 | UserCurrent = &userCurrent | ||
118 | 17 | ) | 18 | ) |
119 | 18 | 19 | ||
120 | 19 | // SetRootCheckFunction allows tests to override the check for a root user. | 20 | // SetRootCheckFunction allows tests to override the check for a root user. |
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: /code.launchpad .net/~axwalk/ juju-core/ local-provider- testability/ +merge/ 204169
https:/
(do not edit description out of merge proposal)
Please review this at https:/ /codereview. appspot. com/58970043/
Affected files (+62, -1 lines): local/environpr ovider. go local/environpr ovider_ test.go local/export_ test.go
A [revision details]
M provider/
M provider/
M provider/
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/environpr ovider. go local/environpr ovider. go' local/environpr ovider. go 2014-01-31 08:44:08 +0000 local/environpr ovider. go 2014-01-31 09:04:29 +0000 s()["namespace" ].(string) ; namespace "%s-%s" , os.Getenv("USER"), cfg.Name()) "%s-%s" , username, cfg.Name()) map[string] interface{ }{"namespace" : namespace})
=== modified file 'provider/
--- provider/
+++ provider/
@@ -51,8 +51,16 @@
// for backwards compatibility: older versions did not store the namespace
// in config.
if namespace, _ := cfg.UnknownAttr
== "" {
+ 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(
+ namespace = fmt.Sprintf(
cfg, err = cfg.Apply(
if err != nil {
return nil, fmt.Errorf("failed to create namespace: %v", err)
Index: provider/ local/environpr ovider_ test.go local/environpr ovider_ test.go' local/environpr ovider_ test.go 2014-01-31 08:44:08 +0000 local/environpr ovider_ test.go 2014-01-31 09:04:29 +0000
=== modified file 'provider/
--- provider/
+++ provider/
@@ -4,6 +4,9 @@
package local_test
import ( com/loggo/ loggo" net/gocheck"
+ "errors"
+ "os/user"
+
"github.
gc "launchpad.
@@ -229,3 +232,50 @@ space(c *gc.C) { local.DetectApt Proxies, func() (osenv. ProxySettings, error) { ings{}, nil New(config. UseDefaults, map[string] interface{ }{ Provider( "local" )
}
}
}
+
+func (s *prepareSuite) TestPrepareName
+ s.PatchValue(
+ return osenv.ProxySett
+ })
+ basecfg, err := config.
+ "type": "local",
+ "name": "test",
+ })
+ provider, err := environs.
+ 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 ...