Merge lp://staging/~niemeyer/juju-core/publish-command into lp://staging/~juju/juju-core/trunk

Reviewers: mp+157460_code.launchpad.net,

Message:
Please take a look.

Description:
cmd/juju: add publish command!

https://code.launchpad.net/~niemeyer/juju-core/publish-command/+merge/157460

Requires:
https://code.launchpad.net/~niemeyer/juju-core/branch-urls/+merge/157098

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/8250045/

Affected files:
   A [revision details]
   M bzr/bzr.go
   M bzr/bzr_test.go
   M charm/repo.go
   M charm/repo_test.go
   M cmd/juju/main.go
   A cmd/juju/publish.go
   A cmd/juju/publish_test.go

A few comments/questions/suggestions -- let me know your thoughts.

https://codereview.appspot.com/8250045/diff/2001/bzr/bzr.go
File bzr/bzr.go (right):

https://codereview.appspot.com/8250045/diff/2001/bzr/bzr.go#newcode140
bzr/bzr.go:140: if bytes.Count(stdout, []byte{'\n'}) == 1 &&
bytes.Contains(stdout, []byte(`See "bzr shelve --list" for details.`)) {
Hmm... if we're depending on output strings, we should probably be
running all the commands in an env with LC_ALL set.

https://codereview.appspot.com/8250045/diff/2001/cmd/juju/main.go
File cmd/juju/main.go (right):

https://codereview.appspot.com/8250045/diff/2001/cmd/juju/main.go#newcode54
cmd/juju/main.go:54: // Creation commands.
Ha, yeah... the fact that we're "Register"ing things is pretty clear ;).

https://codereview.appspot.com/8250045/diff/2001/cmd/juju/publish.go
File cmd/juju/publish.go (right):

https://codereview.appspot.com/8250045/diff/2001/cmd/juju/publish.go#newcode46
cmd/juju/publish.go:46: Purpose: "publish the charm in $PWD to the
store",
I presume there'll be some way to publish from a particular dir other
that $PWD sometime? How do you expect that to look? I'd kinda like
"[<path to charm> [<charm url>]]" args to work but that might get fiddly
-- it's just a suggestion.

And `--from /path/to/charm` or similar would be fine, I think.

https://codereview.appspot.com/8250045/diff/2001/cmd/juju/publish.go#newcode67
cmd/juju/publish.go:67: // Wording guideline to avoid confusion: charms
have *URLs*, branches have *locations*.
Very helpful, thank you.

https://codereview.appspot.com/8250045/diff/2001/cmd/juju/publish.go#newcode70
cmd/juju/publish.go:70: branch := bzr.New(".")
Please run paths through ctx.AbsPath before use. (might simplify testing
a little, too)

https://codereview.appspot.com/8250045/diff/2001/cmd/juju/publish.go#newcode93
cmd/juju/publish.go:93: // out if it can't work without one.
I *think* it does that already, doesn't it? It'll dumbly poke in the
supplied series if required, and then try to parse what it ended up
with, and find that's not valid. The error will not be helpful, though,
and that behaviour is not tested AFAICS.

Possible suggestion: grab environs/config.DefaultSeries and use it here
(and tweak docs to say something like "the following forms will be
accepted and inferred to refer to series %q"?)

https://codereview.appspot.com/8250045/

https://codereview.appspot.com/8250045/diff/2001/bzr/bzr.go
File bzr/bzr.go (right):

https://codereview.appspot.com/8250045/diff/2001/bzr/bzr.go#newcode140
bzr/bzr.go:140: if bytes.Count(stdout, []byte{'\n'}) == 1 &&
bytes.Contains(stdout, []byte(`See "bzr shelve --list" for details.`)) {
On 2013/04/05 21:56:53, fwereade wrote:
> Hmm... if we're depending on output strings, we should probably be
running all
> the commands in an env with LC_ALL set.

Good point. Will do.

https://codereview.appspot.com/8250045/diff/2001/cmd/juju/publish.go
File cmd/juju/publish.go (right):

https://codereview.appspot.com/8250045/diff/2001/cmd/juju/publish.go#newcode46
cmd/juju/publish.go:46: Purpose: "publish the charm in $PWD to the
store",
On 2013/04/05 21:56:53, fwereade wrote:
> And `--from /path/to/charm` or similar would be fine, I think.

Sounds good.

https://codereview.appspot.com/8250045/diff/2001/cmd/juju/publish.go#newcode70
cmd/juju/publish.go:70: branch := bzr.New(".")
On 2013/04/05 21:56:53, fwereade wrote:
> Please run paths through ctx.AbsPath before use. (might simplify
testing a
> little, too)

Sounds good.

https://codereview.appspot.com/8250045/diff/2001/cmd/juju/publish.go#newcode93
cmd/juju/publish.go:93: // out if it can't work without one.
On 2013/04/05 21:56:53, fwereade wrote:
> I *think* it does that already, doesn't it? It'll dumbly poke in the
supplied
> series if required, and then try to parse what it ended up with, and
find that's
> not valid. The error will not be helpful, though, and that behaviour
is not
> tested AFAICS.

> Possible suggestion: grab environs/config.DefaultSeries and use it
here (and
> tweak docs to say something like "the following forms will be accepted
and
> inferred to refer to series %q"?)

We don't have a config here. I think I'd rather preserve this here for
now, including the TODO, if that's okay. It works well for the moment.

https://codereview.appspot.com/8250045/

Please take a look.

https://codereview.appspot.com/8250045/

LGTM, just one point to clear up.

https://codereview.appspot.com/8250045/diff/8001/cmd/juju/publish.go
File cmd/juju/publish.go (right):

https://codereview.appspot.com/8250045/diff/8001/cmd/juju/publish.go#newcode30
cmd/juju/publish.go:30: accepted.
This documentation is now inaccurate, because there's never a default
series and never any inference. The config.DefaultSeries to which I
referred is a package var (const maybe, I forget); it would let you (1)
infer series, and (2) accurately document what series would be inferred.

I'm fine with the simpler behaviour as currently implemented if this is
fixed though.

https://codereview.appspot.com/8250045/

https://codereview.appspot.com/8250045/diff/8001/cmd/juju/publish.go
File cmd/juju/publish.go (right):

https://codereview.appspot.com/8250045/diff/8001/cmd/juju/publish.go#newcode30
cmd/juju/publish.go:30: accepted.
On 2013/04/09 11:30:43, fwereade wrote:
> This documentation is now inaccurate, because there's never a default
series and

Good catch, thanks.

> never any inference. The config.DefaultSeries to which I referred is a
package
> var (const maybe, I forget); it would let you (1) infer series, and
(2)
> accurately document what series would be inferred.

I see. I think this is bogus, though, because it's hardcoded. It means
pushing to "mysql" today can mean something totally different than
pushing to "mysql" tomorrow, just because I upgraded the code.

I'd suggest thinking carefully about where this is being used nowadays.

> I'm fine with the simpler behaviour as currently implemented if this
is fixed
> though.

Sounds good, thanks.

https://codereview.appspot.com/8250045/

Please take a look.

https://codereview.appspot.com/8250045/

LGTM with a bunch of trivials.

https://codereview.appspot.com/8250045/diff/16001/bzr/bzr.go
File bzr/bzr.go (right):

https://codereview.appspot.com/8250045/diff/16001/bzr/bzr.go#newcode32
bzr/bzr.go:32: func cenv() []string {
i'm not too keen on ultra short names, perhaps copyEnv at least?

https://codereview.appspot.com/8250045/diff/16001/bzr/bzr.go#newcode149
bzr/bzr.go:149: func (b *Branch) MustBeClean() error {
"Must" in other parts implies a panic, so maybe "EnsureClean"?

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/main.go
File cmd/juju/main.go (right):

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/main.go#newcode86
cmd/juju/main.go:86: // Charm publishing commands.
shouldn't this be in Common commands instead? It doesn't seem worth
having a group with a single command in it. Will there be other charm
publishing commands?

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/publish.go
File cmd/juju/publish.go (right):

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/publish.go#newcode89
cmd/juju/publish.go:89: return fmt.Errorf("no charm URL provided and
cannot infer from current directory (no push location)")
I think we use lowercase url for errors/logs, but YMMV

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/publish.go#newcode120
cmd/juju/publish.go:120: log.Infof("local digest is %s", localDigest)
badge; see below

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/publish.go#newcode125
cmd/juju/publish.go:125: if _, ok := err.(*charm.NotFoundError); ok {
probably it's worth having charm.IsNotFound() helper for this.

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/publish.go#newcode126
cmd/juju/publish.go:126: log.Infof("charm %s is not yet in the store",
curl)
badge; see below

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/publish.go#newcode138
cmd/juju/publish.go:138: log.Infof("sending charm to the charm
store...")
cmd/juju: ... ? until we remove all log badges i think it's better to
keep them the everywhere.

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/publish_test.go
File cmd/juju/publish_test.go (right):

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/publish_test.go#newcode218
cmd/juju/publish_test.go:218: cmd.SetPollDelay(1e8)
see below

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/publish_test.go#newcode276
cmd/juju/publish_test.go:276: cmd.SetPollDelay(1e8)
see below

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/publish_test.go#newcode332
cmd/juju/publish_test.go:332: cmd.SetPollDelay(1e8)
less cryptic - 1 * time.XX?

https://codereview.appspot.com/8250045/

*** Submitted:

cmd/juju: add publish command!

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

https://codereview.appspot.com/8250045/diff/16001/bzr/bzr.go
File bzr/bzr.go (right):

https://codereview.appspot.com/8250045/diff/16001/bzr/bzr.go#newcode32
bzr/bzr.go:32: func cenv() []string {
On 2013/04/09 15:29:04, dimitern wrote:
> i'm not too keen on ultra short names, perhaps copyEnv at least?

It's a well-documented private function used a single time 10 lines
above, so if you don't mind I'll keep the current name on the basis of
it being a matter of taste only.

https://codereview.appspot.com/8250045/diff/16001/bzr/bzr.go#newcode149
bzr/bzr.go:149: func (b *Branch) MustBeClean() error {
On 2013/04/09 15:29:04, dimitern wrote:
> "Must" in other parts implies a panic, so maybe "EnsureClean"?

Sure. I'll go with CheckClean.

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/main.go
File cmd/juju/main.go (right):

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/main.go#newcode86
cmd/juju/main.go:86: // Charm publishing commands.
On 2013/04/09 15:29:04, dimitern wrote:
> shouldn't this be in Common commands instead? It doesn't seem worth
having a
> group with a single command in it. Will there be other charm
publishing
> commands?

Yes, source.

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/publish.go
File cmd/juju/publish.go (right):

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/publish.go#newcode89
cmd/juju/publish.go:89: return fmt.Errorf("no charm URL provided and
cannot infer from current directory (no push location)")
On 2013/04/09 15:29:04, dimitern wrote:
> I think we use lowercase url for errors/logs, but YMMV

http://paste.ubuntu.com/5692648/

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/publish.go#newcode120
cmd/juju/publish.go:120: log.Infof("local digest is %s", localDigest)
On 2013/04/09 15:29:04, dimitern wrote:
> badge; see below

Done.

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/publish.go#newcode125
cmd/juju/publish.go:125: if _, ok := err.(*charm.NotFoundError); ok {
On 2013/04/09 15:29:04, dimitern wrote:
> probably it's worth having charm.IsNotFound() helper for this.

I don't feel the need, but feel free to propose a change.

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/publish.go#newcode126
cmd/juju/publish.go:126: log.Infof("charm %s is not yet in the store",
curl)
On 2013/04/09 15:29:04, dimitern wrote:
> badge; see below

Done.

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/publish.go#newcode138
cmd/juju/publish.go:138: log.Infof("sending charm to the charm
store...")
On 2013/04/09 15:29:04, dimitern wrote:
> cmd/juju: ... ? until we remove all log badges i think it's better to
keep them
> the everywhere.

Done.

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/publish_test.go
File cmd/juju/publish_test.go (right):

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/publish_test.go#newcode218
cmd/juju/publish_test.go:218: cmd.SetPollDelay(1e8)
On 2013/04/09 15:29:04, dimitern wrote:
> see below

Done.

https://codereview.appspot.com/8250045/diff/16001/cmd/juju/publish_test.go#new...