Add the 'expose' command to the API.

Add the 'expose' command to the API.



https://code.launchpad.net/~bac/juju-core/api-expose/+merge/150648

(do not edit description out of merge proposal)

[bac, 2013-02-27 19:26:16]

Please take a look.

[benji, 2013-02-27 20:23:56]

I don't always review branches for projects that I am not an approved reviewer
for, but when I do it's because I'm trying to get a feel for the project.

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go
File state/api/api_test.go (left):

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go#oldcode224
state/api/api_test.go:224: c.Assert(service, DeepEquals,
&expectedWordpressConfig)
I assume "expectedWordpressConfig" is now only used once.  If so, we could
inline its definition so as not to multiply entities unnecessarily (i.e., having
the expected value at hand outweighs the helpfulness of naming the expected
value).

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go
File state/api/api_test.go (right):

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go#newcode81
state/api/api_test.go:81: allow: []string{"user-admin", "user-other"},
Should there be a "deny" test too?

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go#newcode236
state/api/api_test.go:236: err := st.Client().ServiceExpose("wordpress")
This is outside the scope of this branch, but it would be really great if we
could get away from having to call all the way through the client and server
just to call a simple function.

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go#newcode893
state/api/api_test.go:893: service, err = s.State.Service(serviceName)
Do you have to get the service twice?  Is there some form of snap-shotting going
on that requires this?

https://codereview.appspot.com/7369058/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/7369058/diff/1/state/api/apiclient.go#newcode84
state/api/apiclient.go:84: // nil.
This pattern of restating in English the details of the function declaration is
common in the code base (so I certainly don't blame you for doing as the Romans
do). Seeing this here does make me wonder how beneficial it is.

A DRYer comment might be something like: "Change the juju-managed firewall to
open any ports that were explicitly marked by the units as open."

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose.go
File state/statecmd/expose.go (right):

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose.go#newcode6
state/statecmd/expose.go:6: // API directly.
Do we want to reproduce this in every file under /statecmd/?

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose_test.go
File state/statecmd/expose_test.go (right):

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose_test.go#n...
state/statecmd/expose_test.go:13: var _ = Suite(&ExposeSuite{})
I've been wondering why the tests files do this.  It doesn't look like it does
anything (unless initializing Suite has side-effects, which would be icky).

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose_test.go#n...
state/statecmd/expose_test.go:55: }
The above two functions are a common pattern in the code base that don't
understand.  Two small tests that express the same intent would be simpler and
easier to understand.

[bac, 2013-02-27 20:34:27]

Thanks for the review Benji and the questions it raised.

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go
File state/api/api_test.go (right):

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go#newcode236
state/api/api_test.go:236: err := st.Client().ServiceExpose("wordpress")
Agreed.

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go#newcode893
state/api/api_test.go:893: service, err = s.State.Service(serviceName)
Yes.  Without getting it again .IsExposed() returns stale info.

https://codereview.appspot.com/7369058/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/7369058/diff/1/state/api/apiclient.go#newcode84
state/api/apiclient.go:84: // nil.
I think even the Romans would approve of your suggestion.  Should we ask
Francesco?

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose.go
File state/statecmd/expose.go (right):

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose.go#newcode6
state/statecmd/expose.go:6: // API directly.
If not, where should it be?  It is annoying but if the package really is on
death-row then there is little harm IMO.

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose_test.go
File state/statecmd/expose_test.go (right):

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose_test.go#n...
state/statecmd/expose_test.go:13: var _ = Suite(&ExposeSuite{})
Good question.  Roger?

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose_test.go#n...
state/statecmd/expose_test.go:55: }
I'd like to have a simpler way to do this testing.  It is as if we're recreating
a test runner in each module, which is tiresome and won't scale.

[dimitern, 2013-02-27 21:22:36]

LGTM modulo the suggestions below.

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go
File state/api/api_test.go (right):

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go#newcode223
state/api/api_test.go:223: // This test only test that the call is made without
error, ensuring the
s/test/tests/

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go#newcode236
state/api/api_test.go:236: err := st.Client().ServiceExpose("wordpress")
On 2013/02/27 20:23:56, benji wrote:
> This is outside the scope of this branch, but it would be really great if we
> could get away from having to call all the way through the client and server
> just to call a simple function.

Isn't this the test of the API, so you're supposed to pass through the client
and server. There are separate tests that use the state directly in juju-core,
without using the API server (yet).

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go#newcode893
state/api/api_test.go:893: service, err = s.State.Service(serviceName)
On 2013/02/27 20:34:27, bac wrote:
> Yes.  Without getting it again .IsExposed() returns stale info.
> 

Instead of getting it again, you can just do err = service.Refresh().

https://codereview.appspot.com/7369058/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/7369058/diff/1/state/api/apiclient.go#newcode84
state/api/apiclient.go:84: // nil.
On 2013/02/27 20:23:56, benji wrote:
> This pattern of restating in English the details of the function declaration
is
> common in the code base (so I certainly don't blame you for doing as the
Romans
> do). Seeing this here does make me wonder how beneficial it is.
> 
> A DRYer comment might be something like: "Change the juju-managed firewall to
> open any ports that were explicitly marked by the units as open."

This is the Go's equivalent of Python docstrings - this doc comment has to have
the function name first, in order to be picked up by go doc <module> and appear
in the index.
So all code in juju-core follows this pattern.

https://codereview.appspot.com/7369058/diff/1/state/api/apiserver.go
File state/api/apiserver.go (right):

https://codereview.appspot.com/7369058/diff/1/state/api/apiserver.go#newcode253
state/api/apiserver.go:253: // ServiceGet returns the configuration for a
service or any errors.
s/any errors/an error/

https://codereview.appspot.com/7369058/diff/1/state/api/apiserver.go#newcode259
state/api/apiserver.go:259: // were also explicitly marked by units as open.  It
returns any errors or
ditto

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose.go
File state/statecmd/expose.go (right):

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose.go#newcode6
state/statecmd/expose.go:6: // API directly.
On 2013/02/27 20:34:27, bac wrote:
> If not, where should it be?  It is annoying but if the package really is on
> death-row then there is little harm IMO.

The module comment should match the module's purpose. Hence,  there's no need to
repeat it in every file - in fact, it's confusing. It should be shorter and up
to the point.

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose.go#newcode13
state/statecmd/expose.go:13: // Parameters for making the ServiceExpose call.
// ServiceExposeParams ...

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose_test.go
File state/statecmd/expose_test.go (right):

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose_test.go#n...
state/statecmd/expose_test.go:13: var _ = Suite(&ExposeSuite{})
On 2013/02/27 20:34:27, bac wrote:
> Good question.  Roger?

This is a compile-time check that ExposeSuite implements the Suite interface
fully. It'll fail if it didn't. It's a good thing to have generally (exposing
any omissions earlier).

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose_test.go#n...
state/statecmd/expose_test.go:27: // Exposing a known service succeeds, so err
is "".
I'd either add (part of) this comment into the summary field or drop it
altogether. Uninitialized struct fields always contain a zero-value ("", 0, nil,
etc.).

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose_test.go#n...
state/statecmd/expose_test.go:31: },
How about an additional test case of exposing an exposed service, to make sure
it's sane?

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose_test.go#n...
state/statecmd/expose_test.go:34: func (suite *ExposeSuite) TestServiceExpose(c
*C) {
It's customary to name the suite "s" or something short, unless you have a
compelling reason not to.

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose_test.go#n...
state/statecmd/expose_test.go:48: service, err := suite.State.Service(t.service)
Likewise, you can use service.Refresh() here.

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose_test.go#n...
state/statecmd/expose_test.go:55: }
On 2013/02/27 20:34:27, bac wrote:
> I'd like to have a simpler way to do this testing.  It is as if we're
recreating
> a test runner in each module, which is tiresome and won't scale.
> 

Table based tests are good to express multiple simple cases in a concise manner.
If you're adding new tests it's probably best to add them to the table, but in
this case you can equally well go with a SetUpSuite/Test() code and a couple of
small, separate Test*() cases.
Tables scale pretty well actually - take a look at the uniter tests you you
like.

[bac, 2013-02-28 16:00:01]

Thanks for the review.  I've made all of the changes so far except for adding a
'deny' test.

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go
File state/api/api_test.go (left):

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go#oldcode224
state/api/api_test.go:224: c.Assert(service, DeepEquals,
&expectedWordpressConfig)
On 2013/02/27 20:23:56, benji wrote:
> I assume "expectedWordpressConfig" is now only used once.  If so, we could
> inline its definition so as not to multiply entities unnecessarily (i.e.,
having
> the expected value at hand outweighs the helpfulness of naming the expected
> value).

Done.

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go
File state/api/api_test.go (right):

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go#newcode223
state/api/api_test.go:223: // This test only test that the call is made without
error, ensuring the
On 2013/02/27 21:22:36, dimitern wrote:
> s/test/tests/

Done.

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go#newcode236
state/api/api_test.go:236: err := st.Client().ServiceExpose("wordpress")
The components on the server side are not exported and can't be properly unit
tested as currently written.  So these integration tests are used instead.

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go#newcode893
state/api/api_test.go:893: service, err = s.State.Service(serviceName)
On 2013/02/27 21:22:36, dimitern wrote:
> On 2013/02/27 20:34:27, bac wrote:
> > Yes.  Without getting it again .IsExposed() returns stale info.
> > 
> 
> Instead of getting it again, you can just do err = service.Refresh().

Done.

https://codereview.appspot.com/7369058/diff/1/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/7369058/diff/1/state/api/apiclient.go#newcode84
state/api/apiclient.go:84: // nil.
Thanks for the explanation.

https://codereview.appspot.com/7369058/diff/1/state/api/apiserver.go
File state/api/apiserver.go (right):

https://codereview.appspot.com/7369058/diff/1/state/api/apiserver.go#newcode253
state/api/apiserver.go:253: // ServiceGet returns the configuration for a
service or any errors.
On 2013/02/27 21:22:36, dimitern wrote:
> s/any errors/an error/

Done.

https://codereview.appspot.com/7369058/diff/1/state/api/apiserver.go#newcode259
state/api/apiserver.go:259: // were also explicitly marked by units as open.  It
returns any errors or
On 2013/02/27 21:22:36, dimitern wrote:
> ditto

Done.

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose.go
File state/statecmd/expose.go (right):

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose.go#newcode6
state/statecmd/expose.go:6: // API directly.
On 2013/02/27 21:22:36, dimitern wrote:
> On 2013/02/27 20:34:27, bac wrote:
> > If not, where should it be?  It is annoying but if the package really is on
> > death-row then there is little harm IMO.
> 
> The module comment should match the module's purpose. Hence,  there's no need
to
> repeat it in every file - in fact, it's confusing. It should be shorter and up
> to the point.

Done.

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose.go#newcode13
state/statecmd/expose.go:13: // Parameters for making the ServiceExpose call.
On 2013/02/27 21:22:36, dimitern wrote:
> // ServiceExposeParams ...

Done.

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose_test.go
File state/statecmd/expose_test.go (right):

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose_test.go#n...
state/statecmd/expose_test.go:13: var _ = Suite(&ExposeSuite{})
Thanks, I've added a comment.

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose_test.go#n...
state/statecmd/expose_test.go:27: // Exposing a known service succeeds, so err
is "".
On 2013/02/27 21:22:36, dimitern wrote:
> I'd either add (part of) this comment into the summary field or drop it
> altogether. Uninitialized struct fields always contain a zero-value ("", 0,
nil,
> etc.).

Done.

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose_test.go#n...
state/statecmd/expose_test.go:31: },
On 2013/02/27 21:22:36, dimitern wrote:
> How about an additional test case of exposing an exposed service, to make sure
> it's sane?

Done.

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose_test.go#n...
state/statecmd/expose_test.go:34: func (suite *ExposeSuite) TestServiceExpose(c
*C) {
On 2013/02/27 21:22:36, dimitern wrote:
> It's customary to name the suite "s" or something short, unless you have a
> compelling reason not to.

Done.

https://codereview.appspot.com/7369058/diff/1/state/statecmd/expose_test.go#n...
state/statecmd/expose_test.go:48: service, err := suite.State.Service(t.service)
Actually, the service has to be retrieved by name as it can be one of several.

[bac, 2013-02-28 16:07:38]

Please take a look.

[rog, 2013-02-28 18:42:18]

LGTM with a few trivial suggestions below.

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go
File state/api/api_test.go (right):

https://codereview.appspot.com/7369058/diff/1/state/api/api_test.go#newcode236
state/api/api_test.go:236: err := st.Client().ServiceExpose("wordpress")
On 2013/02/28 16:00:01, bac wrote:
> The components on the server side are not exported and can't be properly unit
> tested as currently written.  So these integration tests are used instead.

we could write internal tests that call the server side functions directly, but
i think i prefer the additional warm glow from having the end-to-end test, even
at the cost of a millisecond or so of overhead.

https://codereview.appspot.com/7369058/diff/12001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/7369058/diff/12001/state/api/apiclient.go#newc...
state/api/apiclient.go:83: // were also explicitly marked by units as open.  It
returns any errors or
the last sentence can be left out - it's implicit when returning an error (if we
documented this for every method, it would be very tedious!)

https://codereview.appspot.com/7369058/diff/12001/state/api/apiserver.go
File state/api/apiserver.go (right):

https://codereview.appspot.com/7369058/diff/12001/state/api/apiserver.go#newc...
state/api/apiserver.go:253: // ServiceGet returns the configuration for a
service or an error.
s/or an error//

https://codereview.appspot.com/7369058/diff/12001/state/api/apiserver.go#newc...
state/api/apiserver.go:259: // were also explicitly marked by units as open.  It
returns an error or
lose the last sentence.

https://codereview.appspot.com/7369058/diff/12001/state/statecmd/expose.go
File state/statecmd/expose.go (right):

https://codereview.appspot.com/7369058/diff/12001/state/statecmd/expose.go#ne...
state/statecmd/expose.go:15: // were also explicitly marked by units as open. 
It returns any errors or
lose the last sentence.

[bac, 2013-02-28 19:04:28]

Thanks Roger

https://codereview.appspot.com/7369058/diff/12001/state/api/apiclient.go
File state/api/apiclient.go (right):

https://codereview.appspot.com/7369058/diff/12001/state/api/apiclient.go#newc...
state/api/apiclient.go:83: // were also explicitly marked by units as open.  It
returns any errors or
On 2013/02/28 18:42:18, rog wrote:
> the last sentence can be left out - it's implicit when returning an error (if
we
> documented this for every method, it would be very tedious!)

Done.

https://codereview.appspot.com/7369058/diff/12001/state/api/apiserver.go
File state/api/apiserver.go (right):

https://codereview.appspot.com/7369058/diff/12001/state/api/apiserver.go#newc...
state/api/apiserver.go:253: // ServiceGet returns the configuration for a
service or an error.
On 2013/02/28 18:42:18, rog wrote:
> s/or an error//

Done.

https://codereview.appspot.com/7369058/diff/12001/state/api/apiserver.go#newc...
state/api/apiserver.go:259: // were also explicitly marked by units as open.  It
returns an error or
On 2013/02/28 18:42:18, rog wrote:
> lose the last sentence.

Done.

https://codereview.appspot.com/7369058/diff/12001/state/statecmd/expose.go
File state/statecmd/expose.go (right):

https://codereview.appspot.com/7369058/diff/12001/state/statecmd/expose.go#ne...
state/statecmd/expose.go:15: // were also explicitly marked by units as open. 
It returns any errors or
On 2013/02/28 18:42:18, rog wrote:
> lose the last sentence.

Done.

[bac, 2013-02-28 21:10:34]

*** Submitted:

Add the 'expose' command to the API.

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