Use stats=0 for charm store interaction by CI

Use stats=0 for charm store interaction by CI

Bug #1276462: CI deployments have a noticable effect on the charm
download statistics.

Unfortunately, just adding an environment variable JUJU_TESTING is not
sufficient: Running something like "JUJU_TESTING=1 juju deploy mysql"
or "JUJU_TESTING juju bootstrap" does not automatically set the
environment variable in the deployed machines, where the charm downloads
happen.

After discussing the problem with TheMue I added instead a new config
parameter "testing". This parameter can then be used on the deployed
machines to add the HTTP query parameter "stats=0" to the URL.

The type Repository has three methods that issue HTTP GET requests to the
store: Get(), Info() and Event(). The latter method is only used during
charm uploads, so I did not change it.

So, Get() and Info() need access to the environment configuration. ISTM
that direct access is not possible in the package charm due to circular
import, so I added a call parameter "testing" to Get() nad Info(); this
parameter is set from the configuration data in the call sites.

https://code.launchpad.net/~adeuring/juju-core/1276462/+merge/208834

(do not edit description out of merge proposal)

[abel.deuring, 2014-02-28 15:10:45]

Please take a look.

[mue, 2014-02-28 15:36:33]

So far mostly OK, but I dislike the identifier "testing" inside the
configuration and the code. It has a special meaning in Go, so I would call it
testMode.

But please talk with fwereade about it, he should review this CL too.

https://codereview.appspot.com/68000045/diff/1/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/68000045/diff/1/charm/repo.go#newcode55
charm/repo.go:55: Latest(testing bool, curls ...*URL) ([]CharmRevision, error)
Yeah, sadly testing has to be set before curls here. Would liked it as last arg
like in Get(), but then ... doesn't work. Same below.

[abel.deuring, 2014-03-03 14:11:48]

Please take a look.

[abel.deuring, 2014-03-07 11:31:58]

On 2014/03/03 14:11:48, abel.deuring wrote:
> Please take a look.

After a discussion with fwereade I removed the parameter testMode from the
methods CharmStore.Get() and CharmStore.Info(). Instead, CharmStoer has a new
field testMode which is set in SpecializeCharmStore(), which was formerly called
AuthorizeCharmStore()

[abel.deuring, 2014-03-07 11:32:46]

Please take a look.

[fwereade, 2014-03-07 12:15:55]

LGTM if the following is clear, ping me if there's any issue

https://codereview.appspot.com/68000045/diff/40001/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/68000045/diff/40001/charm/repo.go#newcode105
charm/repo.go:105: // SetTestMode returns a Repository where testMode is set to
value passed to
please call this WithTestMode, we're not setting anything on the original

https://codereview.appspot.com/68000045/diff/40001/charm/repo.go#newcode143
charm/repo.go:143: charmSnippets := make([]string, len(curls)+1)
revert this line, I don't think we need/want the empty value at the end in
non-test-mode. The following append will still work fine. You *could* do
`make([]string, len(curls), len(curls)+1)` to set capacity, to ensure that the
following append doesn't reallocate -- that'd be quite nice, I guess.

https://codereview.appspot.com/68000045/diff/40001/charm/repo.go#newcode148
charm/repo.go:148: charmSnippets = append(charmSnippets, "stats=0")
"charmSnippets" isn't a great name any more

https://codereview.appspot.com/68000045/diff/40001/charm/repo.go#newcode352
charm/repo.go:352: store_url = store_url + "?stats=0"
I feel like there ought to be library methods for building query strings, and
our complete failure to use them STM to suck a bit. Would you take 5 minutes to
see if you can do something a bit nicer that manual query-building with explicit
url.QueryEscape() calls here and there?

If not, it's not a blocker: I expect there's a reason it's already like this.
But it'd be nice.

https://codereview.appspot.com/68000045/diff/40001/charm/repo_test.go
File charm/repo_test.go (right):

https://codereview.appspot.com/68000045/diff/40001/charm/repo_test.go#newcode223
charm/repo_test.go:223: storeInTestMode, ok :=
s.store.SetTestMode(true).(*charm.CharmStore)
shouldn't need to type assert here, should you?

https://codereview.appspot.com/68000045/diff/40001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/68000045/diff/40001/environs/config/config.go#...
environs/config/config.go:745: "testmode":     false,
"test-mode" please... possible "testing-mode"? Bikeshed, bikeshed, not *really*
bothered, but please be consistent about separating words with "-"

https://codereview.appspot.com/68000045/diff/40001/environs/config/config.go#...
environs/config/config.go:864: repo = CS.SetTestMode(cfg.TestMode())
you could do the auth, authSet check inside here too and only check
.(Specializer) once

https://codereview.appspot.com/68000045/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/68000045/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1733: func (s *clientSuite)
TestClientAuthorizeStoreOnDeployServiceSetCharmAndAddCharm(c *gc.C) {
s/Authorize/Specialize/

[abel.deuring, 2014-03-07 13:59:11]

https://codereview.appspot.com/68000045/diff/40001/charm/repo.go
File charm/repo.go (right):

https://codereview.appspot.com/68000045/diff/40001/charm/repo.go#newcode105
charm/repo.go:105: // SetTestMode returns a Repository where testMode is set to
value passed to
On 2014/03/07 12:15:56, fwereade wrote:
> please call this WithTestMode, we're not setting anything on the original

Done.

https://codereview.appspot.com/68000045/diff/40001/charm/repo.go#newcode143
charm/repo.go:143: charmSnippets := make([]string, len(curls)+1)
On 2014/03/07 12:15:56, fwereade wrote:
> revert this line, I don't think we need/want the empty value at the end in
> non-test-mode. The following append will still work fine. You *could* do
> `make([]string, len(curls), len(curls)+1)` to set capacity, to ensure that the
> following append doesn't reallocate -- that'd be quite nice, I guess.

Done.

https://codereview.appspot.com/68000045/diff/40001/charm/repo.go#newcode148
charm/repo.go:148: charmSnippets = append(charmSnippets, "stats=0")
On 2014/03/07 12:15:56, fwereade wrote:
> "charmSnippets" isn't a great name any more

Changed to queryParams

https://codereview.appspot.com/68000045/diff/40001/charm/repo.go#newcode352
charm/repo.go:352: store_url = store_url + "?stats=0"
On 2014/03/07 12:15:56, fwereade wrote:
> I feel like there ought to be library methods for building query strings, and
> our complete failure to use them STM to suck a bit. Would you take 5 minutes
to
> see if you can do something a bit nicer that manual query-building with
explicit
> url.QueryEscape() calls here and there?
> 
> If not, it's not a blocker: I expect there's a reason it's already like this.
> But it'd be nice.

If I understand the documentation of net/url correctly, QueryEscape() only
replaces characters that are invalid in URL or that have a special "meaning"
into %xx sequences. But I agree that constructing URLs this way sucks. (As well
as calling srings.Join(...) for example in Info(). ISTM that the standard Go
libraries do not have convenient helpers to build HTTP URLs... (But I wouldn't
claim to be very familiar with these libraries...)

https://codereview.appspot.com/68000045/diff/40001/charm/repo_test.go
File charm/repo_test.go (right):

https://codereview.appspot.com/68000045/diff/40001/charm/repo_test.go#newcode223
charm/repo_test.go:223: storeInTestMode, ok :=
s.store.SetTestMode(true).(*charm.CharmStore)
On 2014/03/07 12:15:56, fwereade wrote:
> shouldn't need to type assert here, should you?

No, it's necessary: SetTestMode()/WithTestMode() returns a Repository, not a
CharmStore, like WithAuthAttrs(). But Repository does not define the method
Info(), which is called below.

https://codereview.appspot.com/68000045/diff/40001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/68000045/diff/40001/environs/config/config.go#...
environs/config/config.go:745: "testmode":     false,
On 2014/03/07 12:15:56, fwereade wrote:
> "test-mode" please... possible "testing-mode"? Bikeshed, bikeshed, not
*really*
> bothered, but please be consistent about separating words with "-"

Done.

https://codereview.appspot.com/68000045/diff/40001/environs/config/config.go#...
environs/config/config.go:864: repo = CS.SetTestMode(cfg.TestMode())
On 2014/03/07 12:15:56, fwereade wrote:
> you could do the auth, authSet check inside here too and only check
> .(Specializer) once

well, it's a dance with two types, Repository and CharmStore: Both
WithTestMode() and WithAuthAttrs do not return CharmStore, but we need this
type, not Repository in order to call the methods...

https://codereview.appspot.com/68000045/diff/40001/state/apiserver/client/cli...
File state/apiserver/client/client_test.go (right):

https://codereview.appspot.com/68000045/diff/40001/state/apiserver/client/cli...
state/apiserver/client/client_test.go:1733: func (s *clientSuite)
TestClientAuthorizeStoreOnDeployServiceSetCharmAndAddCharm(c *gc.C) {
On 2014/03/07 12:15:56, fwereade wrote:
> s/Authorize/Specialize/

Done.