worker/uniter/charm: git config user.email on init

worker/uniter/charm: git config user.email on init

If we don't manually set user.email, and git can't
derive an email address from the FQDN (e.g. we don't
have one), then charms cannot be deployed.

Fixes bug #1211629

https://code.launchpad.net/~axwalk/juju-core/lp1211629-git-config-useremail/+merge/180066

(do not edit description out of merge proposal)

[axw, 2013-08-14 08:54:44]

Please take a look.

[natefinch, 2013-08-14 19:07:34]

LGTM

[fwereade, 2013-08-15 09:49:54]

not lgtm without a test, but appears sound otherwise

[axw, 2013-08-15 12:45:35]

Please take a look.

[natefinch, 2013-08-15 13:00:54]

not lgtm given that I'm pretty sure it's not going to compile.

https://codereview.appspot.com/12924043/diff/6001/worker/uniter/charm/git_tes...
File worker/uniter/charm/git_test.go (right):

https://codereview.appspot.com/12924043/diff/6001/worker/uniter/charm/git_tes...
worker/uniter/charm/git_test.go:17: "regexp"
Does this compile? fmt and regexp aren't used as far as I can tell.

Also, can you sort the imports while you're up there?

[axw1, 2013-08-15 13:07:21]

On 2013/08/15 13:00:54, nate.finch wrote:
> not lgtm given that I'm pretty sure it's not going to compile.
> 
>
https://codereview.appspot.com/12924043/diff/6001/worker/uniter/charm/git_tes...
> File worker/uniter/charm/git_test.go (right):
> 
>
https://codereview.appspot.com/12924043/diff/6001/worker/uniter/charm/git_tes...
> worker/uniter/charm/git_test.go:17: "regexp"
> Does this compile? fmt and regexp aren't used as far as I can tell.

Sigh, I need to sleep more. Thanks for the catch... I made a tweak after running
the tests.

> Also, can you sort the imports while you're up there?

Will do.

[axw, 2013-08-15 13:15:02]

Please take a look.

[axw1, 2013-08-15 13:16:09]

On 2013/08/15 13:15:02, axw wrote:
> Please take a look.

Before you tell me, I'll move gc to its own line ;)

[natefinch, 2013-08-15 13:22:29]

On 2013/08/15 13:16:09, axw1 wrote:
> On 2013/08/15 13:15:02, axw wrote:
> > Please take a look.
> 
> Before you tell me, I'll move gc to its own line ;)

Awesome, and glad to see more . imports going away.  LGTM.

[thumper, 2013-08-16 01:10:06]

https://codereview.appspot.com/12924043/diff/13001/worker/uniter/charm/git.go
File worker/uniter/charm/git.go (right):

https://codereview.appspot.com/12924043/diff/13001/worker/uniter/charm/git.go...
worker/uniter/charm/git.go:59: {"config", "user.email", "juju@localhost."},
Why does the email address end in a dot?

https://codereview.appspot.com/12924043/diff/13001/worker/uniter/charm/git_te...
File worker/uniter/charm/git_test.go (left):

https://codereview.appspot.com/12924043/diff/13001/worker/uniter/charm/git_te...
worker/uniter/charm/git_test.go:30: s.oldLcAll = os.Getenv("LC_ALL")
I added a testing.PatchEnvironment function that returns a simple function.

https://codereview.appspot.com/12924043/diff/13001/worker/uniter/charm/git_te...
File worker/uniter/charm/git_test.go (right):

https://codereview.appspot.com/12924043/diff/13001/worker/uniter/charm/git_te...
worker/uniter/charm/git_test.go:12: gc "launchpad.net/gocheck"
We've gone for three sections of imports:

std libs

other imports

juju-core

So please add a blank line after gocheck.

https://codereview.appspot.com/12924043/diff/13001/worker/uniter/charm/git_te...
worker/uniter/charm/git_test.go:52: c.Assert(string(out), gc.Matches,
"(.|\n)*user.email=juju@localhost.\nuser.name=juju(.|\n)*")
In testing/checkers there is a Contains matcher.

c.Assert(string(out), jc.Contains, "user.email=juju@localhost.\n")
c.Assert(string(out), jc.Contains, "user.name=juju\n")

Would probably cast out prior though.

https://codereview.appspot.com/12924043/diff/13001/worker/uniter/charm/git_te...
worker/uniter/charm/git_test.go:60: c.Assert(exists, gc.Equals, false)
There is also IsTrue, and IsFalse in testing/checkers

[axw1, 2013-08-16 01:39:10]

On 2013/08/16 01:10:06, thumper wrote:
> https://codereview.appspot.com/12924043/diff/13001/worker/uniter/charm/git.go
> File worker/uniter/charm/git.go (right):
> 
>
https://codereview.appspot.com/12924043/diff/13001/worker/uniter/charm/git.go...
> worker/uniter/charm/git.go:59: {"config", "user.email", "juju@localhost."},
> Why does the email address end in a dot?

Because I misread the special-use domain RFC. Fixed, though apparently git
doesn't care.

>
https://codereview.appspot.com/12924043/diff/13001/worker/uniter/charm/git_te...
> File worker/uniter/charm/git_test.go (left):
> 
>
https://codereview.appspot.com/12924043/diff/13001/worker/uniter/charm/git_te...
> worker/uniter/charm/git_test.go:30: s.oldLcAll = os.Getenv("LC_ALL")
> I added a testing.PatchEnvironment function that returns a simple function.

Done.

>
https://codereview.appspot.com/12924043/diff/13001/worker/uniter/charm/git_te...
> File worker/uniter/charm/git_test.go (right):
> 
>
https://codereview.appspot.com/12924043/diff/13001/worker/uniter/charm/git_te...
> worker/uniter/charm/git_test.go:12: gc "launchpad.net/gocheck"
> We've gone for three sections of imports:
> 
> std libs
> 
> other imports
> 
> juju-core
> 
> So please add a blank line after gocheck.

Yep sorry, I just missed that in the last one - done. I'm going to write a tool
to check the import groups, and stick it in a bzr pre-commit hook.

>
https://codereview.appspot.com/12924043/diff/13001/worker/uniter/charm/git_te...
> worker/uniter/charm/git_test.go:52: c.Assert(string(out), gc.Matches,
> "(.|\n)*user.email=juju@localhost.\nuser.name=juju(.|\n)*")
> In testing/checkers there is a Contains matcher.
> 
> c.Assert(string(out), jc.Contains, "user.email=juju@localhost.\n")
> c.Assert(string(out), jc.Contains, "user.name=juju\n")
> 
> Would probably cast out prior though.

Done, thanks.

>
https://codereview.appspot.com/12924043/diff/13001/worker/uniter/charm/git_te...
> worker/uniter/charm/git_test.go:60: c.Assert(exists, gc.Equals, false)
> There is also IsTrue, and IsFalse in testing/checkers

Done.

[axw, 2013-08-16 01:40:49]

Please take a look.

[thumper, 2013-08-16 01:51:38]

LGTM with bonus points for updating the import :)

https://codereview.appspot.com/12924043/diff/22001/worker/uniter/charm/git_te...
File worker/uniter/charm/git_test.go (left):

https://codereview.appspot.com/12924043/diff/22001/worker/uniter/charm/git_te...
worker/uniter/charm/git_test.go:11: "launchpad.net/juju-core/testing/checkers"
We have kind of agreed now to use:

jc "launchpad.net/juju-core/testing/checkers"

This ended up being a three way discussion between Roger, David and me, and this
is the compromise.

[thumper, 2013-08-16 01:52:21]

On 2013/08/15 09:49:54, fwereade wrote:
> not lgtm without a test, but appears sound otherwise

Formally overriding this "not merge" as it was just about missing tests, and we
have one now.

[axw1, 2013-08-16 02:59:47]

On 2013/08/16 01:51:38, thumper wrote:
> LGTM with bonus points for updating the import :)
> 
>
https://codereview.appspot.com/12924043/diff/22001/worker/uniter/charm/git_te...
> File worker/uniter/charm/git_test.go (left):
> 
>
https://codereview.appspot.com/12924043/diff/22001/worker/uniter/charm/git_te...
> worker/uniter/charm/git_test.go:11: "launchpad.net/juju-core/testing/checkers"
> We have kind of agreed now to use:
> 
> jc "launchpad.net/juju-core/testing/checkers"
> 
> This ended up being a three way discussion between Roger, David and me, and
this
> is the compromise.

Done.