juju: status should display charm URLs, not names

juju: status should display charm URLs, not names

(Python shows charm URLs, and I think we should match as closely as possible)

https://code.launchpad.net/~fwereade/juju-core/juju-status-charm-display/+merge/141394

(do not edit description out of merge proposal)

[fwereade, 2012-12-28 10:27:46]

Please take a look.

[dimitern, 2013-01-03 22:00:47]

LGTM, just one question.

https://codereview.appspot.com/7013048/diff/1/cmd/juju/status_test.go
File cmd/juju/status_test.go (right):

https://codereview.appspot.com/7013048/diff/1/cmd/juju/status_test.go#newcode132
cmd/juju/status_test.go:132: "charm":   "local:series/dummy-1",
how about testing cs: urls?

[fwereade, 2013-01-04 08:14:30]

On 2013/01/03 22:00:47, dimitern wrote:
> LGTM, just one question.
> 
> https://codereview.appspot.com/7013048/diff/1/cmd/juju/status_test.go
> File cmd/juju/status_test.go (right):
> 
>
https://codereview.appspot.com/7013048/diff/1/cmd/juju/status_test.go#newcode132
> cmd/juju/status_test.go:132: "charm":   "local:series/dummy-1",
> how about testing cs: urls?

I don't see what problems such a test could reasonably be expected to uncover;
and I'm not sure this is the right place to start double-checking what charm
URLs can be. IMO the critical thing is that a name is wrong, and a charm URL is
right, and these tests demonstrate that we do the right thing and not the wrong
one.

[dimitern, 2013-01-04 09:17:07]

On 2013/01/04 08:14:30, fwereade wrote:
> On 2013/01/03 22:00:47, dimitern wrote:
> > LGTM, just one question.
> > 
> > https://codereview.appspot.com/7013048/diff/1/cmd/juju/status_test.go
> > File cmd/juju/status_test.go (right):
> > 
> >
>
https://codereview.appspot.com/7013048/diff/1/cmd/juju/status_test.go#newcode132
> > cmd/juju/status_test.go:132: "charm":   "local:series/dummy-1",
> > how about testing cs: urls?
> 
> I don't see what problems such a test could reasonably be expected to uncover;
> and I'm not sure this is the right place to start double-checking what charm
> URLs can be. IMO the critical thing is that a name is wrong, and a charm URL
is
> right, and these tests demonstrate that we do the right thing and not the
wrong
> one.

OK, the place for such tests is somewhere else, but does my suggestion make
sense - testing known URL formats?

[fwereade, 2013-01-04 10:06:53]

On 2013/01/04 09:17:07, dimitern wrote:
> On 2013/01/04 08:14:30, fwereade wrote:
> > On 2013/01/03 22:00:47, dimitern wrote:
> > > LGTM, just one question.
> > > 
> > > https://codereview.appspot.com/7013048/diff/1/cmd/juju/status_test.go
> > > File cmd/juju/status_test.go (right):
> > > 
> > >
> >
>
https://codereview.appspot.com/7013048/diff/1/cmd/juju/status_test.go#newcode132
> > > cmd/juju/status_test.go:132: "charm":   "local:series/dummy-1",
> > > how about testing cs: urls?
> > 
> > I don't see what problems such a test could reasonably be expected to
uncover;
> > and I'm not sure this is the right place to start double-checking what charm
> > URLs can be. IMO the critical thing is that a name is wrong, and a charm URL
> is
> > right, and these tests demonstrate that we do the right thing and not the
> wrong
> > one.
> 
> OK, the place for such tests is somewhere else, but does my suggestion make
> sense - testing known URL formats?

I *think* that it doesn't, because I don't see how the distinction between
different charm URLs is relevant -- AFAICT the best it can do is verify that we
aren't hardcoding this one specific charm URL. So, well, it's not without *any*
value, but I'm not sure that it's actually worth the effort here.

(This is also, I think, a problem that will *have* to be solved when we do
status properly, and discard all evidence of this cursed dummy charm nonsense --
for now, I only care about making a single baby step in that direction)

[fwereade, 2013-01-04 10:07:48]

On 2013/01/04 10:06:53, fwereade wrote:
> On 2013/01/04 09:17:07, dimitern wrote:
> > On 2013/01/04 08:14:30, fwereade wrote:
> > > On 2013/01/03 22:00:47, dimitern wrote:
> > > > LGTM, just one question.
> > > > 
> > > > https://codereview.appspot.com/7013048/diff/1/cmd/juju/status_test.go
> > > > File cmd/juju/status_test.go (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.appspot.com/7013048/diff/1/cmd/juju/status_test.go#newcode132
> > > > cmd/juju/status_test.go:132: "charm":   "local:series/dummy-1",
> > > > how about testing cs: urls?
> > > 
> > > I don't see what problems such a test could reasonably be expected to
> uncover;
> > > and I'm not sure this is the right place to start double-checking what
charm
> > > URLs can be. IMO the critical thing is that a name is wrong, and a charm
URL
> > is
> > > right, and these tests demonstrate that we do the right thing and not the
> > wrong
> > > one.
> > 
> > OK, the place for such tests is somewhere else, but does my suggestion make
> > sense - testing known URL formats?
> 
> I *think* that it doesn't, because I don't see how the distinction between
> different charm URLs is relevant -- AFAICT the best it can do is verify that
we
> aren't hardcoding this one specific charm URL. So, well, it's not without
*any*
> value, but I'm not sure that it's actually worth the effort here.
> 
> (This is also, I think, a problem that will *have* to be solved when we do
> status properly, and discard all evidence of this cursed dummy charm nonsense
--
> for now, I only care about making a single baby step in that direction)

(Oh, wait, sorry, I think I misunderstood: yes, testing charm URL formats is
entirely sensible, but that is already handled pretty solidly in the charm
package)

[rog, 2013-01-07 09:20:50]

LGTM

[fwereade, 2013-01-07 12:45:37]

*** Submitted:

juju: status should display charm URLs, not names

(Python shows charm URLs, and I think we should match as closely as possible)

R=dimitern, rog
CC=
https://codereview.appspot.com/7013048