state/statecmd: new package

state/statecmd: new package

This is the place where the command implementations in
common between cmd/juju and state/api will live.

In the next CL we'll make state/api call statecmd.SetConfig.

https://code.launchpad.net/~rogpeppe/juju-core/224-api-config-set/+merge/149329

(do not edit description out of merge proposal)

[rog, 2013-02-19 16:45:21]

Please take a look.

[dimitern, 2013-02-19 17:12:01]

Overall LGTM, but I have some questions/comments.

https://codereview.appspot.com/7326052/diff/1001/cmd/juju/config_test.go
File cmd/juju/config_test.go (right):

https://codereview.appspot.com/7326052/diff/1001/cmd/juju/config_test.go#newc...
cmd/juju/config_test.go:114: about: "--config $FILE test",
s/$FILE/file/ ?

https://codereview.appspot.com/7326052/diff/1001/cmd/juju/config_test.go#newc...
cmd/juju/config_test.go:135: c.Check(code, Not(Equals), 0)
Why Check, not Assert?

https://codereview.appspot.com/7326052/diff/1001/cmd/juju/config_test.go#newc...
cmd/juju/config_test.go:138: c.Check(code, Equals, 0)
as above

https://codereview.appspot.com/7326052/diff/1001/cmd/juju/set.go
File cmd/juju/set.go (right):

https://codereview.appspot.com/7326052/diff/1001/cmd/juju/set.go#newcode79
cmd/juju/set.go:79: s := strings.SplitN(o, "=", 2)
strings.Strip(o, " ") first, then split?

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config.go
File state/statecmd/config.go (right):

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config.go#new...
state/statecmd/config.go:41: log.Printf("remove: %q; unvalidated: %q", remove,
unvalidated)
log.Debugf("state/statecmd: ..") ? Or remove altogether.

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config_test.go
File state/statecmd/config_test.go (right):

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config_test.g...
state/statecmd/config_test.go:21: // TODO(rog) make these tests independent of
one another.
perhaps adding a field to set preconditons (like what options are to be set
already) will do the trick?

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config_test.g...
state/statecmd/config_test.go:24: params statecmd.SetConfigParams
comment this field as well?

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config_test.g...
state/statecmd/config_test.go:50: Config: "{}",
if this is valid yaml, make it more obvious, otherwise it looks like json and
it's a bit confusing.

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config_test.g...
state/statecmd/config_test.go:66: Options: map[string]string{
I cannot see how is this different from the foo=bar case above? Is "outlook"
special in some way?

[fwereade, 2013-02-20 08:01:16]

Needs a bit more thought, I'm afraid.

https://codereview.appspot.com/7326052/diff/1001/cmd/juju/config_test.go
File cmd/juju/config_test.go (right):

https://codereview.appspot.com/7326052/diff/1001/cmd/juju/config_test.go#newc...
cmd/juju/config_test.go:135: c.Check(code, Not(Equals), 0)
On 2013/02/19 17:12:01, dimitern wrote:
> Why Check, not Assert?

So we see the results of the following assert, even if the check fails. I might
suggest a Check of stdout as well.

https://codereview.appspot.com/7326052/diff/1001/cmd/juju/set.go
File cmd/juju/set.go (right):

https://codereview.appspot.com/7326052/diff/1001/cmd/juju/set.go#newcode60
cmd/juju/set.go:60: }
Isn't it an error to specify options when a file is already specified?

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config.go
File state/statecmd/config.go (right):

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config.go#new...
state/statecmd/config.go:12: type SetConfigParams struct {
Can we be explicit about this being for a service please? We'll need something
similar for environ config soon enough (I don't think the implementation here
wants to change, the two situations are quite different, but they're both about
setting configs).

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config.go#new...
state/statecmd/config.go:15: Config      string
What's the justification for maintaining parallel paths for options and config?
Won't a map[string]interface{} for Options, containing either strings or the
correct types, do the right thing when later passed into config.Validate?

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config.go#new...
state/statecmd/config.go:41: log.Printf("remove: %q; unvalidated: %q", remove,
unvalidated)
On 2013/02/19 17:12:01, dimitern wrote:
> log.Debugf("state/statecmd: ..") ? Or remove altogether.

+1 to either

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config.go#new...
state/statecmd/config.go:53: validated, err :=
charm.Config().Validate(unvalidated)
I don't think this is correct: we're validating against *some* charm but we have
no guarantee that the service will have the same charm by the time we get around
to writing.

I think this logic needs to move into state so that we can make the write
conditional on the service having the same charm it started with, and retry from
scratch if it changes.

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config_test.go
File state/statecmd/config_test.go (right):

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config_test.g...
state/statecmd/config_test.go:21: // TODO(rog) make these tests independent of
one another.
On 2013/02/19 17:12:01, dimitern wrote:
> perhaps adding a field to set preconditons (like what options are to be set
> already) will do the trick?

+1... we have too many tests like this already, and each one induces crippling
inertia because it's such a hassle to maintain them.

[rog, 2013-02-20 08:43:42]

https://codereview.appspot.com/7326052/diff/1001/cmd/juju/config_test.go
File cmd/juju/config_test.go (right):

https://codereview.appspot.com/7326052/diff/1001/cmd/juju/config_test.go#newc...
cmd/juju/config_test.go:114: about: "--config $FILE test",
On 2013/02/19 17:12:01, dimitern wrote:
> s/$FILE/file/ ?

i'm trying to leave as many things unchanged as reasonable (this CL is about
code movement not bug fixes), so i'll leave it be if that's ok.

https://codereview.appspot.com/7326052/diff/1001/cmd/juju/config_test.go#newc...
cmd/juju/config_test.go:135: c.Check(code, Not(Equals), 0)
On 2013/02/19 17:12:01, dimitern wrote:
> Why Check, not Assert?

because the subsequent test is likely to be more interesting than the results of
an integer comparion.

as a general rule, i prefer to use Check instead of Assert when the result won't
affect subsequent correctness. that means we see more information per failed
test, which i think is a good thing (like it's nice to have a compiler print
more than one error message on failure)

https://codereview.appspot.com/7326052/diff/1001/cmd/juju/config_test.go#newc...
cmd/juju/config_test.go:138: c.Check(code, Equals, 0)
On 2013/02/19 17:12:01, dimitern wrote:
> as above

likewise.

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config.go
File state/statecmd/config.go (right):

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config.go#new...
state/statecmd/config.go:15: Config      string
On 2013/02/20 08:01:16, fwereade wrote:
> What's the justification for maintaining parallel paths for options and
config?
> Won't a map[string]interface{} for Options, containing either strings or the
> correct types, do the right thing when later passed into config.Validate?

the justification is that the gui folks want to provide a way for people to
input settings as YAML, but they don't want to parse the YAML in javascript
(because nobody's written a decent parser in js). if you think this is
insufficient motivation, perhaps we should have a chat with them and see what we
should do.

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config.go#new...
state/statecmd/config.go:41: log.Printf("remove: %q; unvalidated: %q", remove,
unvalidated)
On 2013/02/19 17:12:01, dimitern wrote:
> log.Debugf("state/statecmd: ..") ? Or remove altogether.

yeah, it was there in juju/cmd, but should probably go.

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config.go#new...
state/statecmd/config.go:53: validated, err :=
charm.Config().Validate(unvalidated)
On 2013/02/20 08:01:16, fwereade wrote:
> I don't think this is correct: we're validating against *some* charm but we
have
> no guarantee that the service will have the same charm by the time we get
around
> to writing.
> 
> I think this logic needs to move into state so that we can make the write
> conditional on the service having the same charm it started with, and retry
from
> scratch if it changes.

i agree that the logic should change, but i don't think that this CL is the
right place - the logic is the same as it was previously.

the point of this CL, which i should perhaps have made more obvious in the
description, is to show the gui folks how to factor out the code from juju/cmd
so it can be called from the command line and from the API server.

i agree that the code has bugs - the bugs have been there for a while, and we do
indeed need to fix them, but i would very much prefer to leave the bug fixing
for another CL, if that's ok, and make this one about code movement principally.
otherwise i will not have time today to do the watcher implementation.

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config_test.go
File state/statecmd/config_test.go (right):

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config_test.g...
state/statecmd/config_test.go:21: // TODO(rog) make these tests independent of
one another.
On 2013/02/19 17:12:01, dimitern wrote:
> perhaps adding a field to set preconditons (like what options are to be set
> already) will do the trick?

i'd like to do this, but this CL is all about moving existing functionality from
juju/cmd into state/statecmd.
even though i did come across quite a few bugs along the way, i'd prefer not to
fix them right now, as this isn't what i'm supposed to be doing!

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config_test.g...
state/statecmd/config_test.go:50: Config: "{}",
On 2013/02/19 17:12:01, dimitern wrote:
> if this is valid yaml, make it more obvious, otherwise it looks like json and
> it's a bit confusing.

json is valid yaml. i don't know how to make a yaml map with no elements using
yaml indentation-based format. suggestions?

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config_test.g...
state/statecmd/config_test.go:66: Options: map[string]string{
On 2013/02/19 17:12:01, dimitern wrote:
> I cannot see how is this different from the foo=bar case above? Is "outlook"
> special in some way?

"outlook" is one of the parameters defined by the dummy charm. it's a pity that
it's so far removed from this code.

[fwereade, 2013-02-20 10:41:12]

As discussed online: statecmd is not sensible long term, and must be clearly
documented as such; but until we have a CLI that can use the API, we can't avoid
it. So: acceptable tradeoff for now, but everything in the package must move to
state or state/api as soon as it's practical to do so.

[fwereade, 2013-02-20 10:42:02]

On 2013/02/20 10:41:12, fwereade wrote:
> As discussed online: statecmd is not sensible long term, and must be clearly
> documented as such; but until we have a CLI that can use the API, we can't
avoid
> it. So: acceptable tradeoff for now, but everything in the package must move
to
> state or state/api as soon as it's practical to do so.

So: LGTM, and I agree that fixing the bugs is not your task today :). If any
aren't recorded inLP, though, please add them.

[rog, 2013-02-20 11:30:49]

Please take a look.

https://codereview.appspot.com/7326052/diff/1001/cmd/juju/set.go
File cmd/juju/set.go (right):

https://codereview.appspot.com/7326052/diff/1001/cmd/juju/set.go#newcode60
cmd/juju/set.go:60: }
On 2013/02/20 08:01:16, fwereade wrote:
> Isn't it an error to specify options when a file is already specified?

yes, but that's caught in Init above.

https://codereview.appspot.com/7326052/diff/1001/cmd/juju/set.go#newcode79
cmd/juju/set.go:79: s := strings.SplitN(o, "=", 2)
On 2013/02/19 17:12:01, dimitern wrote:
> strings.Strip(o, " ") first, then split?

i don't think so - spaces aren't special here, and someone might want to set a
value that ends in a space.

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config.go
File state/statecmd/config.go (right):

https://codereview.appspot.com/7326052/diff/1001/state/statecmd/config.go#new...
state/statecmd/config.go:12: type SetConfigParams struct {
On 2013/02/20 08:01:16, fwereade wrote:
> Can we be explicit about this being for a service please? We'll need something
> similar for environ config soon enough (I don't think the implementation here
> wants to change, the two situations are quite different, but they're both
about
> setting configs).

renamed to ServiceSet.

[rog, 2013-02-20 12:08:38]

*** Submitted:

state/statecmd: new package

This is the place where the command implementations in
common between cmd/juju and state/api will live.

In the next CL we'll make state/api call statecmd.SetConfig.

R=dimitern, fwereade
CC=
https://codereview.appspot.com/7326052