cmd/juju: add expose

cmd/juju: add expose

Adds juju expose command, also normalises the format of
commadn descriptions (no capital letters, no trailing period).

https://code.launchpad.net/~dave-cheney/juju-core/go-cmd-juju-expose/+merge/118324

(do not edit description out of merge proposal)

[dfc, 2012-08-06 09:48:02]

Please take a look.

[fwereade, 2012-08-06 11:04:40]

Couple of suggestions

https://codereview.appspot.com/6458085/diff/4001/cmd/juju/expose.go
File cmd/juju/expose.go (right):

https://codereview.appspot.com/6458085/diff/4001/cmd/juju/expose.go#newcode49
cmd/juju/expose.go:49: service, err := st.Service(c.ServiceName)
I'm pretty sure this should be done through Conn directly/

https://codereview.appspot.com/6458085/diff/4001/cmd/juju/expose_test.go
File cmd/juju/expose_test.go (right):

https://codereview.appspot.com/6458085/diff/4001/cmd/juju/expose_test.go#newc...
cmd/juju/expose_test.go:39: func (s *ExposeSuite) TestAddUnit(c *C) {
TestExposed :)

https://codereview.appspot.com/6458085/diff/4001/cmd/juju/expose_test.go#newc...
cmd/juju/expose_test.go:48: s.assertExposed(c, "some-service-name")
Maybe a test or two for bad/missing service names?

[dfc, 2012-08-06 21:52:50]

Please take a look.

https://codereview.appspot.com/6458085/diff/4001/cmd/juju/expose.go
File cmd/juju/expose.go (right):

https://codereview.appspot.com/6458085/diff/4001/cmd/juju/expose.go#newcode49
cmd/juju/expose.go:49: service, err := st.Service(c.ServiceName)
On 2012/08/06 11:04:40, fwereade wrote:
> I'm pretty sure this should be done through Conn directly/

Done. I've added conn.Expose(string)

https://codereview.appspot.com/6458085/diff/4001/cmd/juju/expose_test.go
File cmd/juju/expose_test.go (right):

https://codereview.appspot.com/6458085/diff/4001/cmd/juju/expose_test.go#newc...
cmd/juju/expose_test.go:39: func (s *ExposeSuite) TestAddUnit(c *C) {
On 2012/08/06 11:04:40, fwereade wrote:
> TestExposed :)

Done.

https://codereview.appspot.com/6458085/diff/4001/cmd/juju/expose_test.go#newc...
cmd/juju/expose_test.go:48: s.assertExposed(c, "some-service-name")
On 2012/08/06 11:04:40, fwereade wrote:
> Maybe a test or two for bad/missing service names?

Done.

[dfc, 2012-08-07 01:42:58]

Please take a look.

[fwereade, 2012-08-07 07:31:05]

LGTM

[niemeyer, 2012-08-07 11:17:49]

https://codereview.appspot.com/6458085/diff/4001/cmd/juju/expose.go
File cmd/juju/expose.go (right):

https://codereview.appspot.com/6458085/diff/4001/cmd/juju/expose.go#newcode49
cmd/juju/expose.go:49: service, err := st.Service(c.ServiceName)
On 2012/08/06 21:52:50, dfc wrote:
> On 2012/08/06 11:04:40, fwereade wrote:
> > I'm pretty sure this should be done through Conn directly/
> 
> Done. I've added conn.Expose(string)

It should take a *Service rather than a string, similarly to how the rest of the
interface is structured.

This change isn't showing in this branch, though, so I'm not sure that the CL
reflects what you intend to be reviewed and pushed? I'm putting it back onto WIP
for that reason.

[niemeyer, 2012-08-07 13:01:47]

LGTM

https://codereview.appspot.com/6458085/diff/4003/juju/expose.go
File juju/expose.go (right):

https://codereview.appspot.com/6458085/diff/4003/juju/expose.go#newcode3
juju/expose.go:3: // Expose exposes a service to the internet.
As we discussed, I'm not sure there's much benefit in having this here, but I
don't mind too much in this instance as we have an actual command with the same
name.

If you decide to keep it, I suggest documentation similar to:

// Expose changes the juju-managed firewall to expose any
// ports that were also explicitly marked by units as open.

And the service argument should be a *Service.

https://codereview.appspot.com/6458085/diff/4003/juju/expose_test.go
File juju/expose_test.go (right):

https://codereview.appspot.com/6458085/diff/4003/juju/expose_test.go#newcode15
juju/expose_test.go:15: func (s *ExposeSuite) SetUpTest(c *C) {
Are these necessary?

https://codereview.appspot.com/6458085/diff/4003/juju/expose_test.go#newcode31
juju/expose_test.go:31: err = s.conn.Expose("testriak")
The test should check that the service is actually exposed via the state API.

[dfc, 2012-08-08 02:12:06]

*** Submitted:

cmd/juju: add expose

Adds juju expose command, also normalises the format of
commadn descriptions (no capital letters, no trailing period).

R=fwereade, niemeyer
CC=
https://codereview.appspot.com/6458085

https://codereview.appspot.com/6458085/diff/4003/juju/expose.go
File juju/expose.go (right):

https://codereview.appspot.com/6458085/diff/4003/juju/expose.go#newcode3
juju/expose.go:3: // Expose exposes a service to the internet.
Thank you for your comments. As discussed on juju-dev there is no clear
consensus on the utility of conn.State(). In light of that I have rolled this
functionality back into cmd/juju/expose.go directly.