https://codereview.appspot.com/11321043/diff/1/environs/local/config.go
File environs/local/config.go (right):

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode16
environs/local/config.go:16: var rootCheckFunction = func() bool {
On 2013/07/16 13:30:01, rog wrote:
> On 2013/07/16 06:55:50, jameinel wrote:
> > On 2013/07/16 03:56:35, wallyworld wrote:
> > > Perhaps checkRoot() is better?
> >
> >
> > checkIfRoot ?

> isRoot? (or even "amRoot")


Went with checkIfRoot

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode17
environs/local/config.go:17: return os.Getuid() == 0
On 2013/07/16 14:14:35, mue wrote:
> Any need why this is a variable?

Yes, we need to override it for tests.

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode81
environs/local/config.go:81: // if the values have been set.  If the
values haven't been set, then
On 2013/07/16 13:30:01, rog wrote:
> this reads slightly ambiguously to me.

> perhaps:

> // getSudoCallerIds returns the user id and group id of the SUDO
caller.
> // If either is unset, it returns zero for both values.
> // An error is returned if the relevant environment variables
> // are not valid integers.
> ?

Shamelessly copied.

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode84
environs/local/config.go:84: func getSudoCallerIds() (uid, gid int, err
error) {
On 2013/07/16 13:30:01, rog wrote:
> sudoCallerIds ?

You really don't like "get" do you?

Renamed.

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode86
environs/local/config.go:86: // change ownership of the directories.
On 2013/07/16 03:56:35, wallyworld wrote:
> I don't think the above comment belongs here

No it isn't.  Deleted.

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode92
environs/local/config.go:92: logger.Errorf("expected %q for SUDO_UID to
be an int: %v", uidStr, err)
On 2013/07/16 13:30:01, rog wrote:
> do we need to log this error here? wouldn't
> it be better to return a better error which
> will be surely be logged later?

Cleaned up like below.

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode98
environs/local/config.go:98: uid = 0 // clear out any value we may have
On 2013/07/16 14:14:35, mue wrote:
> On 2013/07/16 13:30:01, rog wrote:
> > i think that rather than messing around assigning to the return
values,
> > i'd do:
> >
> > if uidStr == "" || gidStr == "" {
> >     return 0, 0, nil
> > }
> > uid, err := strconv.Atoi(...)
> > if err != nil {
> >    return 0, 0, fmt.Errorf("invalid value %q for SUDO_UID", gidStr)
> > }
> > gid, err := stdconv.Atoi(...)
> > if err != nil {
> >    return 0, 0, fmt.Errorf("similar...")
> > }
> > return uid, gid, nil
> >
> > this keeps things a bit more obvious, with better error messages
> > to boot (the error from Atoi is rarely useful)

> +1

Much nicer.

https://codereview.appspot.com/11321043/diff/1/environs/local/config.go#newcode126
environs/local/config.go:126: if info.IsDir() && err == nil {
On 2013/07/16 13:30:01, rog wrote:
> if err == nil && info.IsDir() {

> (if there's an error, the info may be nil)

> although in fact, i think:

> if info != nil && info.IsDir() {

> is probably better still.
> not that any of this is likely to happen running as root.

Done.

https://codereview.appspot.com/11321043/diff/1/environs/local/config_test.go
File environs/local/config_test.go (right):

https://codereview.appspot.com/11321043/diff/1/environs/local/config_test.go#newcode87
environs/local/config_test.go:87: defer
local.SetRootCheckFunction(rootCheck)
On 2013/07/16 06:55:50, jameinel wrote:
> I would probably label this origRootCheck or something to that effect,
so it is
> immediately clear that this was the value before setting it without
having to
> dig up the return value of SetRootCheckFunction.

Done.

https://codereview.appspot.com/11321043/diff/1/environs/local/export_test.go
File environs/local/export_test.go (right):

https://codereview.appspot.com/11321043/diff/1/environs/local/export_test.go#newcode17
environs/local/export_test.go:17: }
On 2013/07/16 06:55:50, jameinel wrote:
> This is fine as is, though for testing stuff I do still prefer the
style of

> return func() {
>   rootCheckFunction = old
> }

> But if you ever want to inspect what the real root check does, I
suppose this is
> fine, too.

Still getting used to passing around lambdas.  I think that returning
the restore function is a good idea.

https://codereview.appspot.com/11321043/