|
|
|
Resolve series with charm store in juju client.
For the deploy and upgradecharm commands, when a series is not provided, and a
default-series is not set in the environment config, the client will resolve
the series with the charm store, through the state server. If the client is
working with a 1.16 state server, it will resolve the series directly with the
charm store.
Existing environments will have a default-series, so they should have no
change in series selection.
New environments will continue to support the default-series config setting,
but it can be omitted, and is not set by default.
https://code.launchpad.net/~cmars/juju-core/resolve-cs-series/+merge/212755
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 8
Patch Set 2 : Resolve series with charm store in juju client. #
Total comments: 16
Patch Set 3 : Resolve series with charm store in juju client. #Patch Set 4 : Resolve series with charm store in juju client. #
Total comments: 16
Patch Set 5 : Resolve series with charm store in juju client. #Patch Set 6 : Resolve series with charm store in juju client. #Patch Set 7 : Resolve series with charm store in juju client. #
Total comments: 23
Patch Set 8 : Resolve series with charm store in juju client. #
Total comments: 1
Total messages: 21
|
cmars
Please take a look.
|
11 years, 9 months ago (2014年03月26日 02:00:14 UTC) #1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Please take a look.
This is the client-side to support the LTS transition to trusty (LP: #1290824). It can't land until the charm store update (LP: #1290828) is deployed into production (expected tomorrow). In the meantime, I'd much appreciate your feedback & review. Thanks, Casey
I think we have some common code that should be factored out, and I think we need to consider how the code will operate with older Juju server versions, but otherwise I think this looks pretty good. https://codereview.appspot.com/80280043/diff/1/cmd/juju/deploy.go File cmd/juju/deploy.go (right): https://codereview.appspot.com/80280043/diff/1/cmd/juju/deploy.go#newcode155 cmd/juju/deploy.go:155: repo, err := charm.InferRepository(ref, ctx.AbsPath(c.RepoPath)) This feels like stuff that definitely needs to be done, but shouldn't be done in "cmd/juju/deploy.go" which is a "main" sort of function. Is there a reason this can't be part of more "library" code and tested as such? Specifically it feels like it should be a function that takes the command line parameter (CharmName) a client and a conf, and returns a fully qualified charm.URL (or error). Especially given that you essentially implemented it 2 times in this file. (at least the code you added below this looks a lot like the code you added above this). https://codereview.appspot.com/80280043/diff/1/cmd/juju/upgradecharm.go File cmd/juju/upgradecharm.go (left): https://codereview.appspot.com/80280043/diff/1/cmd/juju/upgradecharm.go#oldco... cmd/juju/upgradecharm.go:208: } And here, though this one seems to have a new SpecializeCharmRepo step that I didn't see before. https://codereview.appspot.com/80280043/diff/1/cmd/juju/upgradecharm.go File cmd/juju/upgradecharm.go (right): https://codereview.appspot.com/80280043/diff/1/cmd/juju/upgradecharm.go#newco... cmd/juju/upgradecharm.go:148: } And implemented again here? https://codereview.appspot.com/80280043/diff/1/state/api/client.go File state/api/client.go (right): https://codereview.appspot.com/80280043/diff/1/state/api/client.go#newcode573 state/api/client.go:573: We need to also add code to handle the fallback case. What do we do if you are using juju 1.18 to deploy against a 1.16 server that doesn't have the ResolveCharm API. I'm guessing the answer is to print an error of: "You must supply a series in your charm URL for juju < 1.18". Either that, or we see that the ResolveCharm isn't available, and just issue a warning with something like "server version is too old to support ResolveCharm (juju <1.18) falling back to default series of "precise"" To be clear, that compatibility code probably shouldn't be here. But probably could be in the common helper I was outlining earlier.
PTAL. If everything looks ok, I'd still like to run a few more tests against the deployed charm store update tomorrow, before landing. Thanks, Casey https://codereview.appspot.com/80280043/diff/1/cmd/juju/deploy.go File cmd/juju/deploy.go (right): https://codereview.appspot.com/80280043/diff/1/cmd/juju/deploy.go#newcode155 cmd/juju/deploy.go:155: repo, err := charm.InferRepository(ref, ctx.AbsPath(c.RepoPath)) On 2014年03月26日 07:11:17, jameinel wrote: > This feels like stuff that definitely needs to be done, but shouldn't be done in > "cmd/juju/deploy.go" which is a "main" sort of function. > Is there a reason this can't be part of more "library" code and tested as such? > > Specifically it feels like it should be a function that takes the command line > parameter (CharmName) a client and a conf, and returns a fully qualified > charm.URL (or error). > > Especially given that you essentially implemented it 2 times in this file. (at > least the code you added below this looks a lot like the code you added above > this). Good call, I've refactored functions to cmd/common.go, shared among deploy and upgradecharm. https://codereview.appspot.com/80280043/diff/1/cmd/juju/upgradecharm.go File cmd/juju/upgradecharm.go (left): https://codereview.appspot.com/80280043/diff/1/cmd/juju/upgradecharm.go#oldco... cmd/juju/upgradecharm.go:208: } On 2014年03月26日 07:11:17, jameinel wrote: > And here, though this one seems to have a new SpecializeCharmRepo step that I > didn't see before. Done. https://codereview.appspot.com/80280043/diff/1/cmd/juju/upgradecharm.go File cmd/juju/upgradecharm.go (right): https://codereview.appspot.com/80280043/diff/1/cmd/juju/upgradecharm.go#newco... cmd/juju/upgradecharm.go:148: } On 2014年03月26日 07:11:17, jameinel wrote: > And implemented again here? Done. https://codereview.appspot.com/80280043/diff/1/state/api/client.go File state/api/client.go (right): https://codereview.appspot.com/80280043/diff/1/state/api/client.go#newcode573 state/api/client.go:573: On 2014年03月26日 07:11:17, jameinel wrote: > We need to also add code to handle the fallback case. What do we do if you are > using juju 1.18 to deploy against a 1.16 server that doesn't have the > ResolveCharm API. > > I'm guessing the answer is to print an error of: > "You must supply a series in your charm URL for juju < 1.18". > > Either that, or we see that the ResolveCharm isn't available, and just issue a > warning with something like "server version is too old to support ResolveCharm > (juju <1.18) falling back to default series of "precise"" > > To be clear, that compatibility code probably shouldn't be here. But probably > could be in the common helper I was outlining earlier. > > I decided to warn and fall back on "precise" for <1.18 state servers. These existing environments will almost certainly already have a default-series set by their bootstrapping. I had originally intended to resolve the series directly with the charm store for that case (since 1.16 hits the charm store directly for other things), but it seems unlikely that such an environment would benefit from it.
Please take a look.
The necessary charm store updates to support this change have been deployed to store.juju.ubuntu.com. It is safe to land this branch now, waiting for your review.
A few issues -- don't think any of them should be too much hassle to resolve, the DefaultSeries return value and the API bulk call thing should both be pretty mechanical. https://codereview.appspot.com/80280043/diff/20001/cmd/juju/common.go File cmd/juju/common.go (right): https://codereview.appspot.com/80280043/diff/20001/cmd/juju/common.go#newcode82 cmd/juju/common.go:82: logger.Warningf(`ResolveCharm not supported by the API server, falling back to default series "precise".`) We should in fact be guaranteed a value given a 1.16 state server, but I heartily endorse this behaviour all the same. https://codereview.appspot.com/80280043/diff/20001/environs/config/config.go File environs/config/config.go (left): https://codereview.appspot.com/80280043/diff/20001/environs/config/config.go#... environs/config/config.go:411: func (c *Config) DefaultSeries() string { In-band errors squick me out... now that it's possible to have no default-series, please add a ,ok return value. And I guess return false if it *is* set, but is set to "". https://codereview.appspot.com/80280043/diff/20001/state/api/client.go File state/api/client.go (right): https://codereview.appspot.com/80280043/diff/20001/state/api/client.go#newcod... state/api/client.go:614: args := params.CharmURL{URL: ref.String()} mmmmm I don't quite like that... can we make this a separate type? params.CharmURL now means something different (although I'm surprised, a little... I could have sworn that charm url fields marshalled to strings without any hassle at all [0], and so I'd expect us to be using actual *charm.URLs, and to have to add a new type for a reference anyway.) [0] yeah, they should do, they have MarshalJSON and UnmarshalJSON. Wonder why wedon't make use of it... https://codereview.appspot.com/80280043/diff/20001/state/api/client.go#newcod... state/api/client.go:616: if err := c.st.Call("Client", "", "ResolveCharm", args, result); err != nil { Bulk calls please, they don't have to be exposed in api.Client but it's reasonable to expect to be able to resolve a few urls in one go, and we should accommodate that in the interface. https://codereview.appspot.com/80280043/diff/20001/state/apiserver/client/cli... File state/apiserver/client/client.go (right): https://codereview.appspot.com/80280043/diff/20001/state/apiserver/client/cli... state/apiserver/client/client.go:959: ref, series, err := charm.ParseReference(args.URL) yeah, calling it URL is jarring -- new type OK? https://codereview.appspot.com/80280043/diff/20001/state/apiserver/client/cli... File state/apiserver/client/client_test.go (right): https://codereview.appspot.com/80280043/diff/20001/state/apiserver/client/cli... state/apiserver/client/client_test.go:1977: store.DefaultSeries = t.defaultSeries set this outside loop? https://codereview.appspot.com/80280043/diff/20001/state/apiserver/client/cli... state/apiserver/client/client_test.go:1979: comment := gc.Commentf("defaultSeries:%s charmName:%s", t.defaultSeries, t.charmName) for i, test := range tests { c.Logf("test %d: %#v", i, test) ...is a reasonably compact way to make the test logs somewhat useful. I do really like calling it "test", not "t", though :). https://codereview.appspot.com/80280043/diff/20001/state/apiserver/client/cli... state/apiserver/client/client_test.go:1984: c.Assert(err, gc.IsNil, comment) // All of these should parse can we Check and continue, here and below? check returns a bool (false on failure iirc?), and while one piece of test data may have a problem I think it's good to carry on and run the others.
Please take a look.
https://codereview.appspot.com/80280043/diff/20001/environs/config/config.go File environs/config/config.go (left): https://codereview.appspot.com/80280043/diff/20001/environs/config/config.go#... environs/config/config.go:411: func (c *Config) DefaultSeries() string { On 2014年03月28日 10:43:11, fwereade wrote: > In-band errors squick me out... now that it's possible to have no > default-series, please add a ,ok return value. And I guess return false if it > *is* set, but is set to "". Done. https://codereview.appspot.com/80280043/diff/20001/state/api/client.go File state/api/client.go (right): https://codereview.appspot.com/80280043/diff/20001/state/api/client.go#newcod... state/api/client.go:614: args := params.CharmURL{URL: ref.String()} On 2014年03月28日 10:43:11, fwereade wrote: > mmmmm I don't quite like that... can we make this a separate type? > params.CharmURL now means something different (although I'm surprised, a > little... I could have sworn that charm url fields marshalled to strings without > any hassle at all [0], and so I'd expect us to be using actual *charm.URLs, and > to have to add a new type for a reference anyway.) > > [0] yeah, they should do, they have MarshalJSON and UnmarshalJSON. Wonder why > wedon't make use of it... Done. https://codereview.appspot.com/80280043/diff/20001/state/api/client.go#newcod... state/api/client.go:616: if err := c.st.Call("Client", "", "ResolveCharm", args, result); err != nil { On 2014年03月28日 10:43:11, fwereade wrote: > Bulk calls please, they don't have to be exposed in api.Client but it's > reasonable to expect to be able to resolve a few urls in one go, and we should > accommodate that in the interface. Done. https://codereview.appspot.com/80280043/diff/20001/state/apiserver/client/cli... File state/apiserver/client/client.go (right): https://codereview.appspot.com/80280043/diff/20001/state/apiserver/client/cli... state/apiserver/client/client.go:959: ref, series, err := charm.ParseReference(args.URL) On 2014年03月28日 10:43:11, fwereade wrote: > yeah, calling it URL is jarring -- new type OK? Done. https://codereview.appspot.com/80280043/diff/20001/state/apiserver/client/cli... File state/apiserver/client/client_test.go (right): https://codereview.appspot.com/80280043/diff/20001/state/apiserver/client/cli... state/apiserver/client/client_test.go:1977: store.DefaultSeries = t.defaultSeries On 2014年03月28日 10:43:11, fwereade wrote: > set this outside loop? Varying the default series in the mock charm store among the test conditions helps ensure the value isn't being hard-coded anywhere in the apiserver, and it's useful for simulating a failure to resolve. https://codereview.appspot.com/80280043/diff/20001/state/apiserver/client/cli... state/apiserver/client/client_test.go:1979: comment := gc.Commentf("defaultSeries:%s charmName:%s", t.defaultSeries, t.charmName) On 2014年03月28日 10:43:11, fwereade wrote: > for i, test := range tests { > c.Logf("test %d: %#v", i, test) > > ...is a reasonably compact way to make the test logs somewhat useful. > > I do really like calling it "test", not "t", though :). Done. https://codereview.appspot.com/80280043/diff/20001/state/apiserver/client/cli... state/apiserver/client/client_test.go:1984: c.Assert(err, gc.IsNil, comment) // All of these should parse On 2014年03月28日 10:43:11, fwereade wrote: > can we Check and continue, here and below? check returns a bool (false on > failure iirc?), and while one piece of test data may have a problem I think it's > good to carry on and run the others. Done.
Please take a look.
Nearly there -- a quibble with the location of the PreferredSeries code, and a bit of work on the API. https://codereview.appspot.com/80280043/diff/20001/state/apiserver/client/cli... File state/apiserver/client/client_test.go (right): https://codereview.appspot.com/80280043/diff/20001/state/apiserver/client/cli... state/apiserver/client/client_test.go:1977: store.DefaultSeries = t.defaultSeries On 2014年03月31日 19:57:07, cmars wrote: > On 2014年03月28日 10:43:11, fwereade wrote: > > set this outside loop? > > Varying the default series in the mock charm store among the test conditions > helps ensure the value isn't being hard-coded anywhere in the apiserver, and > it's useful for simulating a failure to resolve. Ofc, thanks. I think my eye slipped over the `t.` -- but it's much harder to miss the `test.`. Thanks :). https://codereview.appspot.com/80280043/diff/60001/cmd/juju/addmachine.go File cmd/juju/addmachine.go (right): https://codereview.appspot.com/80280043/diff/60001/cmd/juju/addmachine.go#new... cmd/juju/addmachine.go:129: series = conf.PreferredSeries() mm, I rather liked the PreferredSeries(conf) spelling, especially if it were using an interface with just the DefaultSeries method. This doesn't feel fundamental to a config -- does that seem sane? https://codereview.appspot.com/80280043/diff/60001/cmd/juju/common.go File cmd/juju/common.go (right): https://codereview.appspot.com/80280043/diff/60001/cmd/juju/common.go#newcode82 cmd/juju/common.go:82: logger.Warningf(`ResolveCharm not supported by the API server, falling back to default series "precise".`) PreferredSeries should surely be guaranteed to return non-""? https://codereview.appspot.com/80280043/diff/60001/environs/config/config.go File environs/config/config.go (right): https://codereview.appspot.com/80280043/diff/60001/environs/config/config.go#... environs/config/config.go:230: return DefaultSeries How do we determine the value of this? I'm feeling like it really ought to actually *be* the latest LTS, rather than just some global var set by who-knows-who. https://codereview.appspot.com/80280043/diff/60001/state/api/params/params.go File state/api/params/params.go (right): https://codereview.appspot.com/80280043/diff/60001/state/api/params/params.go... state/api/params/params.go:338: URLs []charm.URL I think we need an error per-result here, don't we? https://codereview.appspot.com/80280043/diff/60001/state/apiserver/client/cli... File state/apiserver/client/client.go (right): https://codereview.appspot.com/80280043/diff/60001/state/apiserver/client/cli... state/apiserver/client/client.go:963: return params.ResolveCharmResults{}, err sorry for the hassle, but we should always return one result per request, and that result should contain either the answer or the error. Client is a dog's dinner in this regard, and can only gradually and incrementally improve, but the internal APIs are generally written as I want them, and should be used as a model. The core idea is that bulk APIs can be used for single calls, but single APIs can't be used in bulk; as humans we are bad at predicting the future, and I'd rather just do everything bulk-style so as to avoid needless churn. https://codereview.appspot.com/80280043/diff/60001/state/apiserver/client/cli... File state/apiserver/client/client_test.go (right): https://codereview.appspot.com/80280043/diff/60001/state/apiserver/client/cli... state/apiserver/client/client_test.go:1980: comment := gc.Commentf("defaultSeries:%s charmName:%s", test.defaultSeries, test.charmName) This comment should be pretty much redundant now, with the logging. Break a test and look at the output with/without the comment -- and then use whichever you prefer, but do look at them :). https://codereview.appspot.com/80280043/diff/60001/state/apiserver/client/cli... state/apiserver/client/client_test.go:2158: c.Assert(machines[2].Series(), gc.Equals, "non-default") would be pretty nice to dupe this test with an empty default-series. I know it's not your code originally, but... :)
Thanks for reviewing. Couple of questions: https://codereview.appspot.com/80280043/diff/60001/cmd/juju/addmachine.go File cmd/juju/addmachine.go (right): https://codereview.appspot.com/80280043/diff/60001/cmd/juju/addmachine.go#new... cmd/juju/addmachine.go:129: series = conf.PreferredSeries() On 2014年04月01日 07:10:07, fwereade wrote: > mm, I rather liked the PreferredSeries(conf) spelling, especially if it were > using an interface with just the DefaultSeries method. This doesn't feel > fundamental to a config -- does that seem sane? Will do. Should this live in environs/config or a different package? https://codereview.appspot.com/80280043/diff/60001/environs/config/config.go File environs/config/config.go (right): https://codereview.appspot.com/80280043/diff/60001/environs/config/config.go#... environs/config/config.go:230: return DefaultSeries On 2014年04月01日 07:10:07, fwereade wrote: > How do we determine the value of this? I'm feeling like it really ought to > actually *be* the latest LTS, rather than just some global var set by > who-knows-who. I can try to get this from 'distro-info --lts'. If that fails to exec, I'll use a hard-coded fallback (named as such). What do you think?
Please take a look.
I've addressed most of the feedback, but there is a problem lurking. When config.LatestLtsSeries() == "trusty", some of the tests are failing due to tools availability. I was able to resolve some of them, but not all. Could really use some advice. To reproduce the failures, this will simulate a post-trusty release scenario in the proposed branch: go test -ldflags "-X launchpad.net/juju-core/environs/config.latestLtsSeries trusty" ./... These are the failures: FAIL: bootstrap_test.go:506: BootstrapSuite.TestAutoUploadAfterFailedSync FAIL: bootstrap_test.go:555: BootstrapSuite.TestMissingToolsUploadFailedError FAIL: upgradejuju_test.go:301: UpgradeJujuSuite.TestUpgradeJuju FAIL: imagemetadata_test.go:141: ImageMetadataSuite.TestImageMetadataFilesDefaultSeries Everything passes when the latest LTS is precise. Please advise, Casey https://codereview.appspot.com/80280043/diff/60001/cmd/juju/addmachine.go File cmd/juju/addmachine.go (right): https://codereview.appspot.com/80280043/diff/60001/cmd/juju/addmachine.go#new... cmd/juju/addmachine.go:129: series = conf.PreferredSeries() On 2014年04月01日 07:10:07, fwereade wrote: > mm, I rather liked the PreferredSeries(conf) spelling, especially if it were > using an interface with just the DefaultSeries method. This doesn't feel > fundamental to a config -- does that seem sane? Done. https://codereview.appspot.com/80280043/diff/60001/cmd/juju/common.go File cmd/juju/common.go (right): https://codereview.appspot.com/80280043/diff/60001/cmd/juju/common.go#newcode82 cmd/juju/common.go:82: logger.Warningf(`ResolveCharm not supported by the API server, falling back to default series "precise".`) On 2014年04月01日 07:10:07, fwereade wrote: > PreferredSeries should surely be guaranteed to return non-""? Done. https://codereview.appspot.com/80280043/diff/60001/environs/config/config.go File environs/config/config.go (right): https://codereview.appspot.com/80280043/diff/60001/environs/config/config.go#... environs/config/config.go:230: return DefaultSeries On 2014年04月01日 15:05:12, cmars wrote: > On 2014年04月01日 07:10:07, fwereade wrote: > > How do we determine the value of this? I'm feeling like it really ought to > > actually *be* the latest LTS, rather than just some global var set by > > who-knows-who. > > I can try to get this from 'distro-info --lts'. If that fails to exec, I'll use > a hard-coded fallback (named as such). What do you think? Done. https://codereview.appspot.com/80280043/diff/60001/state/api/params/params.go File state/api/params/params.go (right): https://codereview.appspot.com/80280043/diff/60001/state/api/params/params.go... state/api/params/params.go:338: URLs []charm.URL On 2014年04月01日 07:10:07, fwereade wrote: > I think we need an error per-result here, don't we? Done. https://codereview.appspot.com/80280043/diff/60001/state/apiserver/client/cli... File state/apiserver/client/client.go (right): https://codereview.appspot.com/80280043/diff/60001/state/apiserver/client/cli... state/apiserver/client/client.go:963: return params.ResolveCharmResults{}, err On 2014年04月01日 07:10:07, fwereade wrote: > sorry for the hassle, but we should always return one result per request, and > that result should contain either the answer or the error. > > Client is a dog's dinner in this regard, and can only gradually and > incrementally improve, but the internal APIs are generally written as I want > them, and should be used as a model. The core idea is that bulk APIs can be used > for single calls, but single APIs can't be used in bulk; as humans we are bad at > predicting the future, and I'd rather just do everything bulk-style so as to > avoid needless churn. Done. https://codereview.appspot.com/80280043/diff/60001/state/apiserver/client/cli... File state/apiserver/client/client_test.go (right): https://codereview.appspot.com/80280043/diff/60001/state/apiserver/client/cli... state/apiserver/client/client_test.go:1980: comment := gc.Commentf("defaultSeries:%s charmName:%s", test.defaultSeries, test.charmName) On 2014年04月01日 07:10:07, fwereade wrote: > This comment should be pretty much redundant now, with the logging. Break a test > and look at the output with/without the comment -- and then use whichever you > prefer, but do look at them :). Done. https://codereview.appspot.com/80280043/diff/60001/state/apiserver/client/cli... state/apiserver/client/client_test.go:2158: c.Assert(machines[2].Series(), gc.Equals, "non-default") On 2014年04月01日 07:10:07, fwereade wrote: > would be pretty nice to dupe this test with an empty default-series. I know it's > not your code originally, but... :) I'm not quite clear on where I'd set it.
Please take a look.
Tests affected by the LTS change wrt uploading tools are now all passing when the latest LTS series is "trusty". To run the tests as if trusty were already released, run with: go test -ldflags "-X launchpad.net/juju-core/environs/config.latestLtsSeries trusty" (This var is normally initialized when not set, from `distro-info --lts`) Added wallyworld. I could use a review from a tools distribution perspective, as this is an area I am only superficially familiar with. They were affected by the LTS series change so I've tried to make fixes where test cases and setup had "precise" hardcoded, did not anticipate having to upload tools other than a DefaultSeries, etc. Thanks! -Casey
Please take a look.
I think the tools changes are ok. What I like is that when we start running the tests on a trusty host, the test set up will, where relevant, continue to correctly use a series matching the host of which the tests are running, hence replicating the current test behaviour. Having said that, I still fear our test coverage of the various permutations in this area is not complete, but that's outside the scope of this branch. There's still the possibility of perhaps a subtle bug being introduced with regard to series handling, as has been seen before when this sort of this was tweaked, but it's hard to know for sure.
Looks good, would appreciate at least a short chat before landing -- in particular, I'm not quite clear on the forces that lead us to use FakeDefaultSeries sometimes, and LatestLts at others. https://codereview.appspot.com/80280043/diff/120001/cmd/juju/bootstrap_test.go File cmd/juju/bootstrap_test.go (right): https://codereview.appspot.com/80280043/diff/120001/cmd/juju/bootstrap_test.g... cmd/juju/bootstrap_test.go:123: useVersion := strings.Replace(test.version, "%LTS%", config.LatestLtsSeries(), 1) 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 File cmd/juju/upgradecharm.go (right): https://codereview.appspot.com/80280043/diff/120001/cmd/juju/upgradecharm.go#... cmd/juju/upgradecharm.go:209: repo, err := charm.InferRepository(newURL.Reference, c.RepoPath) 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? https://codereview.appspot.com/80280043/diff/120001/environs/config/config.go File environs/config/config.go (right): https://codereview.appspot.com/80280043/diff/120001/environs/config/config.go... environs/config/config.go:106: yeah, this feels a bit tacked-on here. Let's give it its own package if we don't think of anywhere better. https://codereview.appspot.com/80280043/diff/120001/environs/config/config.go... environs/config/config.go:456: series := s.(string) this is guaranteed to be a string if it's got this far, right? https://codereview.appspot.com/80280043/diff/120001/provider/common/bootstrap.go File provider/common/bootstrap.go (right): https://codereview.appspot.com/80280043/diff/120001/provider/common/bootstrap... provider/common/bootstrap.go:45: selectedTools, err := EnsureBootstrapTools(ctx, env, config.PreferredSeries(env.Config()), cons.Arch) quibble quibble, we should probably allow bootstrap on any series we can find tools for. Out of scope today, but maybe worth a bug? https://codereview.appspot.com/80280043/diff/120001/provider/ec2/ec2.go File provider/ec2/ec2.go (right): https://codereview.appspot.com/80280043/diff/120001/provider/ec2/ec2.go#newco... provider/ec2/ec2.go:367: Series: config.PreferredSeries(e.ecfg()), this seems a bit weird, but so did the original. let it stand. https://codereview.appspot.com/80280043/diff/120001/provider/ec2/live_test.go File provider/ec2/live_test.go (right): https://codereview.appspot.com/80280043/diff/120001/provider/ec2/live_test.go... provider/ec2/live_test.go:104: Series: coretesting.FakeDefaultSeries, wondering if this should be LatestLts? I presume you have a good reason it isn't, but it's not immediately clear ;) 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#newco... state/api/client.go:613: func (c *Client) ResolveCharms(refs ...charm.Reference) ([]params.ResolveCharmResult, error) { 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. https://codereview.appspot.com/80280043/diff/120001/state/api/params/params.go File state/api/params/params.go (right): https://codereview.appspot.com/80280043/diff/120001/state/api/params/params.g... state/api/params/params.go:335: References []charm.Reference remind me, does this jsonify to a string? https://codereview.appspot.com/80280043/diff/120001/state/apiserver/client/cl... File state/apiserver/client/client.go (right): https://codereview.appspot.com/80280043/diff/120001/state/apiserver/client/cl... state/apiserver/client/client.go:576: prefSeries = config.PreferredSeries(conf) is it rational to move all this block... https://codereview.appspot.com/80280043/diff/120001/state/apiserver/client/cl... state/apiserver/client/client.go:595: p.Series = prefSeries ...into here? and not hit the db for config unless it's really needed? https://codereview.appspot.com/80280043/diff/120001/state/apiserver/client/cl... state/apiserver/client/client.go:985: return store.Resolve(ref) I suspect it would be nicer to move this stuff into ResolveCharm to avoid the repeated db hits
PTAL https://codereview.appspot.com/80280043/diff/120001/cmd/juju/bootstrap_test.go File cmd/juju/bootstrap_test.go (right): https://codereview.appspot.com/80280043/diff/120001/cmd/juju/bootstrap_test.g... 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... LP: #1301999 https://codereview.appspot.com/80280043/diff/120001/cmd/juju/upgradecharm.go File cmd/juju/upgradecharm.go (right): https://codereview.appspot.com/80280043/diff/120001/cmd/juju/upgradecharm.go#... 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/environs/config/config.go File environs/config/config.go (right): https://codereview.appspot.com/80280043/diff/120001/environs/config/config.go... environs/config/config.go:106: 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... environs/config/config.go:456: series := s.(string) 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 File provider/common/bootstrap.go (right): https://codereview.appspot.com/80280043/diff/120001/provider/common/bootstrap... 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? LP: #1302005 https://codereview.appspot.com/80280043/diff/120001/provider/ec2/live_test.go File provider/ec2/live_test.go (right): https://codereview.appspot.com/80280043/diff/120001/provider/ec2/live_test.go... provider/ec2/live_test.go:104: Series: coretesting.FakeDefaultSeries, 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#newco... 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. > Done. https://codereview.appspot.com/80280043/diff/120001/state/api/params/params.go File state/api/params/params.go (right): https://codereview.appspot.com/80280043/diff/120001/state/api/params/params.g... state/api/params/params.go:335: References []charm.Reference 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/cl... File state/apiserver/client/client.go (right): https://codereview.appspot.com/80280043/diff/120001/state/apiserver/client/cl... state/apiserver/client/client.go:576: prefSeries = config.PreferredSeries(conf) 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/cl... state/apiserver/client/client.go:595: p.Series = prefSeries 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/cl... state/apiserver/client/client.go:985: return store.Resolve(ref) 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.
LGTM with one trivial. Thanks for this, it's a monster diff -- let's throw it at CI and see what we've missed ;). https://codereview.appspot.com/80280043/diff/140001/charm/url_test.go File charm/url_test.go (right): https://codereview.appspot.com/80280043/diff/140001/charm/url_test.go#newcode292 charm/url_test.go:292: c.Check(parsed, gc.DeepEquals, ref) add one for unmarshalling gibberish, just for safety's sake