https://codereview.appspot.com/80280043/diff/120001/cmd/juju/bootstrap_test.go#newcode123
cmd/juju/bootstrap_test.go:123: useVersion :=
strings.Replace(test.version, "%LTS%", config.LatestLtsSeries(), 1)
On 2014/04/03 13:34:15, fwereade wrote:
> not quite sure that config is the right package for these -- but I'm
not sure I
> can think of a better one. Unless you want to create a tiny,
hyper-focused,
> series package somewhere? I'm sure it'll grow excitingly as we deal
with other
> OSs...
https://codereview.appspot.com/80280043/diff/120001/cmd/juju/upgradecharm.go#newcode209
cmd/juju/upgradecharm.go:209: repo, err :=
charm.InferRepository(newURL.Reference, c.RepoPath)
On 2014/04/03 13:34:15, fwereade wrote:
> We still need ctx.AbsPath, I think. Would you change the relevant test
to use a
> path relative to the working dir so we check this properly please?
Ah, must have botched this in a prior merge. Restored it.
https://codereview.appspot.com/80280043/diff/120001/provider/common/bootstrap.go#newcode45
provider/common/bootstrap.go:45: selectedTools, err :=
EnsureBootstrapTools(ctx, env, config.PreferredSeries(env.Config()),
cons.Arch)
On 2014/04/03 13:34:15, fwereade wrote:
> quibble quibble, we should probably allow bootstrap on any series we
can find
> tools for. Out of scope today, but maybe worth a bug?
It's to separate what is used for default-series: in tests from the
preferred series selection. This allows us to test scenarios against
different configured series vs. what will be preferred.
Most of what's affected atm is the tools setup & bootstrapping for some
tests. There are still some areas lurking where its hard-coded to
precise.
https://codereview.appspot.com/80280043/diff/120001/state/api/client.go#newcode613
state/api/client.go:613: func (c *Client) ResolveCharms(refs
...charm.Reference) ([]params.ResolveCharmResult, error) {
On 2014/04/03 13:34:15, fwereade wrote:
> I think this client-side method might be cleaner as non-bulk -- I
don't really
> care about bulk calls and compatibility for state/api, just for
state/apiserver.
PTAL
https:/ /codereview. appspot. com/80280043/ diff/120001/ cmd/juju/ bootstrap_ test.go bootstrap_ test.go (right):
File cmd/juju/
https:/ /codereview. appspot. com/80280043/ diff/120001/ cmd/juju/ bootstrap_ test.go# newcode123 bootstrap_ test.go: 123: useVersion := Replace( test.version, "%LTS%", config. LatestLtsSeries (), 1)
cmd/juju/
strings.
On 2014/04/03 13:34:15, fwereade wrote:
> not quite sure that config is the right package for these -- but I'm
not sure I
> can think of a better one. Unless you want to create a tiny,
hyper-focused,
> series package somewhere? I'm sure it'll grow excitingly as we deal
with other
> OSs...
LP: #1301999
https:/ /codereview. appspot. com/80280043/ diff/120001/ cmd/juju/ upgradecharm. go upgradecharm. go (right):
File cmd/juju/
https:/ /codereview. appspot. com/80280043/ diff/120001/ cmd/juju/ upgradecharm. go#newcode209 upgradecharm. go:209: repo, err := sitory( newURL. Reference, c.RepoPath)
cmd/juju/
charm.InferRepo
On 2014/04/03 13:34:15, fwereade wrote:
> We still need ctx.AbsPath, I think. Would you change the relevant test
to use a
> path relative to the working dir so we check this properly please?
Ah, must have botched this in a prior merge. Restored it.
https:/ /codereview. appspot. com/80280043/ diff/120001/ environs/ config/ config. go config/ config. go (right):
File environs/
https:/ /codereview. appspot. com/80280043/ diff/120001/ environs/ config/ config. go#newcode106 config/ config. go:106:
environs/
On 2014/04/03 13:34:15, fwereade wrote:
> yeah, this feels a bit tacked-on here. Let's give it its own package
if we don't
> think of anywhere better.
LP: #1301999
https:/ /codereview. appspot. com/80280043/ diff/120001/ environs/ config/ config. go#newcode456 config/ config. go:456: series := s.(string)
environs/
On 2014/04/03 13:34:15, fwereade wrote:
> this is guaranteed to be a string if it's got this far, right?
Non-string values that make their way in here will be ignored with a
warning.
https:/ /codereview. appspot. com/80280043/ diff/120001/ provider/ common/ bootstrap. go common/ bootstrap. go (right):
File provider/
https:/ /codereview. appspot. com/80280043/ diff/120001/ provider/ common/ bootstrap. go#newcode45 common/ bootstrap. go:45: selectedTools, err := Tools(ctx, env, config. PreferredSeries (env.Config( )),
provider/
EnsureBootstrap
cons.Arch)
On 2014/04/03 13:34:15, fwereade wrote:
> quibble quibble, we should probably allow bootstrap on any series we
can find
> tools for. Out of scope today, but maybe worth a bug?
LP: #1302005
https:/ /codereview. appspot. com/80280043/ diff/120001/ provider/ ec2/live_ test.go ec2/live_ test.go (right):
File provider/
https:/ /codereview. appspot. com/80280043/ diff/120001/ provider/ ec2/live_ test.go# newcode104 ec2/live_ test.go: 104: Series: coretesting. FakeDefaultSeri es,
provider/
On 2014/04/03 13:34:15, fwereade wrote:
> wondering if this should be LatestLts? I presume you have a good
reason it
> isn't, but it's not immediately clear ;)
It's to separate what is used for default-series: in tests from the
preferred series selection. This allows us to test scenarios against
different configured series vs. what will be preferred.
Most of what's affected atm is the tools setup & bootstrapping for some
tests. There are still some areas lurking where its hard-coded to
precise.
I've opened LP: #1302015 to address this.
https:/ /codereview. appspot. com/80280043/ diff/120001/ state/api/ client. go
File state/api/client.go (right):
https:/ /codereview. appspot. com/80280043/ diff/120001/ state/api/ client. go#newcode613 client. go:613: func (c *Client) ResolveCharms(refs ResolveCharmRes ult, error) {
state/api/
...charm.Reference) ([]params.
On 2014/04/03 13:34:15, fwereade wrote:
> I think this client-side method might be cleaner as non-bulk -- I
don't really
> care about bulk calls and compatibility for state/api, just for
state/apiserver.
Done.
https:/ /codereview. appspot. com/80280043/ diff/120001/ state/api/ params/ params. go params/ params. go (right):
File state/api/
https:/ /codereview. appspot. com/80280043/ diff/120001/ state/api/ params/ params. go#newcode335 params/ params. go:335: References []charm.Reference
state/api/
On 2014/04/03 13:34:15, fwereade wrote:
> remind me, does this jsonify to a string?
It does now.
https:/ /codereview. appspot. com/80280043/ diff/120001/ state/apiserver /client/ client. go /client/ client. go (right):
File state/apiserver
https:/ /codereview. appspot. com/80280043/ diff/120001/ state/apiserver /client/ client. go#newcode576 /client/ client. go:576: prefSeries = PreferredSeries (conf)
state/apiserver
config.
On 2014/04/03 13:34:15, fwereade wrote:
> is it rational to move all this block...
Done.
https:/ /codereview. appspot. com/80280043/ diff/120001/ state/apiserver /client/ client. go#newcode595 /client/ client. go:595: p.Series = prefSeries
state/apiserver
On 2014/04/03 13:34:15, fwereade wrote:
> ...into here? and not hit the db for config unless it's really needed?
Done.
https:/ /codereview. appspot. com/80280043/ diff/120001/ state/apiserver /client/ client. go#newcode985 /client/ client. go:985: return store.Resolve(ref)
state/apiserver
On 2014/04/03 13:34:15, fwereade wrote:
> I suspect it would be nicer to move this stuff into ResolveCharm to
avoid the
> repeated db hits
Done.
https:/ /codereview. appspot. com/80280043/