Rollup of all relevent changes on lp:gomaasapi

Rollup of all relevent changes on lp:gomaasapi

This is for review only, showing the current trunk of the project in
the diff (from the initial commit of the licence and a couple of stub
go files). Note it's not possible for the Red squad to update this
proposal directly, so don't expect code changes in response to comments
to appear here, please watch for other merge proposals. :)

https://code.launchpad.net/~gz/gomaasapi/gomaasapi/+merge/145811

(do not edit description out of merge proposal)

[gz, 2013-01-31 10:49:04]

Please take a look.

[TheMue, 2013-02-01 10:43:41]

Mostly a good start. So far only

- some missing comments,
- sometimes non-go-typical identifiers and
- would like more explicit oauth tests.

Only major item is the JSONObject. Looks too complicated and heavy. Also using
strings as types instead of 

const (
    StringType = iota
    IntType
    ...
)

Why isn't the direct usage of JSON serialization not usable here?

https://codereview.appspot.com/7228069/diff/1/client.go
File client.go (right):

https://codereview.appspot.com/7228069/diff/1/client.go#newcode15
client.go:15: type Client struct {
Please add godoc documentation on all public types and functions.

https://codereview.appspot.com/7228069/diff/1/client.go#newcode24
client.go:24: func (client Client) dispatchRequest(request *http.Request)
([]byte, error) {
Here (c Client) is more usual.

https://codereview.appspot.com/7228069/diff/1/client.go#newcode35
client.go:35: if response.StatusCode/100 != 2 {
It's working, but I would like 

response.StatusCode >= 200 && response.StatusCode <= 299

more. It's more clear what's tested here.

https://codereview.appspot.com/7228069/diff/1/client.go#newcode36
client.go:36: return body, errors.New("Error requesting the MAAS server: " +
response.Status + ".")
return nil, fmt.Errorf("gomaasapi: cannot dispatch request: %v",
response.Status)

In case of an error no value is returned.

[rog, 2013-02-01 17:30:05]

seems like a good start.

lots of minor comments, but one or two more significant queries. in particular,
it looks like it's only possible to use the maas package if you know exactly how
maas works. i'd like to see all the gruesome http details hidden behind an API
that actually reflects the operations and entities that maas provides.

i'm +1 on TheMue's comment on JSONObject. it seems like it's a lot of code that
doesn't solve any real problem.

https://codereview.appspot.com/7228069/diff/1/client.go
File client.go (right):

https://codereview.appspot.com/7228069/diff/1/client.go#newcode15
client.go:15: type Client struct {
On 2013/02/01 10:43:41, TheMue wrote:
> Please add godoc documentation on all public types and functions.

+1
also please unexport any methods that aren't essential to using the package.

https://codereview.appspot.com/7228069/diff/1/client.go#newcode21
client.go:21: operationParamName = "op"
this seems a very long name for a very short string :-)

https://codereview.appspot.com/7228069/diff/1/client.go#newcode24
client.go:24: func (client Client) dispatchRequest(request *http.Request)
([]byte, error) {
On 2013/02/01 10:43:41, TheMue wrote:
> Here (c Client) is more usual.

further to that, i'd suggest that on type with side effects, it's conventional
to use pointer types, so you'd use *Client throughout.

https://codereview.appspot.com/7228069/diff/1/client.go#newcode26
client.go:26: httpClient := http.Client{}
unless there's a specific reason not to, it would be more conventional to use
http.DefaultClient here rather than creating a new instance of http.Client. that
way connections can be reused.

https://codereview.appspot.com/7228069/diff/1/client.go#newcode36
client.go:36: return body, errors.New("Error requesting the MAAS server: " +
response.Status + ".")
On 2013/02/01 10:43:41, TheMue wrote:
> return nil, fmt.Errorf("gomaasapi: cannot dispatch request: %v",
> response.Status)
> 
> In case of an error no value is returned.

+1

but we wouldn't usually include the package name in the error message although
the Go core would. either is acceptable.

https://codereview.appspot.com/7228069/diff/1/client.go#newcode54
client.go:54: queryUrl := client.GetURL(URI)
the distinction between "URI" and "URL" is unclear to me here. perhaps some
documentation might make things more clear?

also, we tend to use lower case names for variables, so "uri" rather than "URI".

https://codereview.appspot.com/7228069/diff/1/client.go#newcode66
client.go:66: request, err := http.NewRequest(method, URL.String(),
strings.NewReader(string(parameters.Encode())))
the string conversion on the result of Encode is unnecessary here.

https://codereview.appspot.com/7228069/diff/1/client.go#newcode90
client.go:90: _, err2 := client.dispatchRequest(request)
s/err2/err/
no real need to make another variable.

https://codereview.appspot.com/7228069/diff/1/client.go#newcode104
client.go:104: var _ OAuthSigner = (*anonSigner)(nil)
given that you're using a by-value method above, this may as well be:

var _ OAuthSigner = anonSigner{}

https://codereview.appspot.com/7228069/diff/1/client.go#newcode121
client.go:121: errString := "Invalid API key. The format of the key must be
\"<consumer secret>:<token key>:<token secret>\"."
return nil, fmt.Errorf(`invalid API key %q; expected "<consumer secret>:<token
key>:<token secret>"`, apiKey)

might be more conventional (error strings don't usually begin with capital
letters, as they often form part of a larger error message).

https://codereview.appspot.com/7228069/diff/1/client.go#newcode128
client.go:128: ConsumerSecret: "",
no need to have explicit initialisation for zero-valued fields.

https://codereview.appspot.com/7228069/diff/1/example/live_example.go
File example/live_example.go (right):

https://codereview.appspot.com/7228069/diff/1/example/live_example.go#newcode20
example/live_example.go:20: func init() {
this is cute, but probably better to call a function at the start of main, as
the order that init function execute in is unspecified.

https://codereview.appspot.com/7228069/diff/1/example/live_example.go#newcode23
example/live_example.go:23: fmt.Scanf("%s", &apiKey)
you should check the error from scanf. if the user presses ^D, you want the
program to stop.

https://codereview.appspot.com/7228069/diff/1/example/live_example.go#newcode25
example/live_example.go:25: fmt.Scanf("%s", &apiURL)
ditto.

https://codereview.appspot.com/7228069/diff/1/example/live_example.go#newcode34
example/live_example.go:34: maas, err := gomaasapi.NewMAAS(*authClient)
if err != nil {
  panic
}

https://codereview.appspot.com/7228069/diff/1/example/live_example.go#newcode44
example/live_example.go:44: listNodes, err := listNodeObjects.GetArray()
if err != nil {
   panic
}

alternatively you could have a little function:

func checkError(err error) {
   if err != nil {
       panic(err)
   }
}

to save a little bit of space in a program where it doesn't matter too much.

https://codereview.appspot.com/7228069/diff/1/example/live_example.go#newcode47
example/live_example.go:47: node, _ := nodeObj.GetMAASObject()
is this ignoring an error?

https://codereview.appspot.com/7228069/diff/1/example/live_example.go#newcode48
example/live_example.go:48: hostname, _ := node.GetField("hostname")
ditto.

https://codereview.appspot.com/7228069/diff/1/example/live_example.go#newcode55
example/live_example.go:55: newNodeObj, err := nodeListing.CallPost("new",
params)
this seems like a very low-level way to interact with a MAAS server. surely we
can do things at a higher level than http requests?

https://codereview.appspot.com/7228069/diff/1/example/live_example.go#newcode59
example/live_example.go:59: newNode, _ := newNodeObj.GetMAASObject()
please check errors here (if appropriate) and below.
it's not a great example if it doesn't, because any real code will need to.

https://codereview.appspot.com/7228069/diff/1/example/live_example.go#newcode81
example/live_example.go:81: errDelete := newNode.Delete()
if err := newNode.Delete(); err != nil {
    panic(err)
}

or checkError as suggest above.

https://codereview.appspot.com/7228069/diff/1/jsonobject.go
File jsonobject.go (right):

https://codereview.appspot.com/7228069/diff/1/jsonobject.go#newcode32
jsonobject.go:32: GetString() (string, error)
i don't really see what these methods have to offer over:

s, ok := someValue.(string)

https://codereview.appspot.com/7228069/diff/1/jsonobject.go#newcode73
jsonobject.go:73: switch value.(type) {
switch value := value.(type) {

then value will have the appropriate type in each arm of the switch.

https://codereview.appspot.com/7228069/diff/1/jsonobject.go#newcode76
jsonobject.go:76: case int:
this case will never happen - json.Unmarshal always unmarshals to float64 by
default.

https://codereview.appspot.com/7228069/diff/1/jsonobject.go#newcode117
jsonobject.go:117: func failConversion(wanted_type string, obj JSONObject) error
{
camel case is more usual for variable names (wantedType)

https://codereview.appspot.com/7228069/diff/1/jsonobject.go#newcode119
jsonobject.go:119: return errors.New(msg)
fmt.Errorf("cannot convert %#v to %s", obj, wanted_type)

https://codereview.appspot.com/7228069/diff/1/jsonobject_test.go
File jsonobject_test.go (right):

https://codereview.appspot.com/7228069/diff/1/jsonobject_test.go#newcode11
jsonobject_test.go:11: func (suite *GomaasapiTestSuite) TestMaasifyConvertsNil(c
*C) {
as a general pattern it's quite nice to have each file have its own test suite,
rather than using a single test suite across the whole package.

then it's easy to see if there are fixtures, and also easy to move test suites
between packages if desired.

https://codereview.appspot.com/7228069/diff/1/maasobject.go
File maasobject.go (right):

https://codereview.appspot.com/7228069/diff/1/maasobject.go#newcode16
maasobject.go:16: type MAASObject interface {
this doesn't feel right to me. i'd expect to see specific types for given kinds
of API objects, with fields that hold information that is specific to that API
object.

https://codereview.appspot.com/7228069/diff/1/oauth.go
File oauth.go (right):

https://codereview.appspot.com/7228069/diff/1/oauth.go#newcode18
oauth.go:18: rand.Seed(time.Now().UTC().UnixNano())
see below.

https://codereview.appspot.com/7228069/diff/1/oauth.go#newcode22
oauth.go:22: return strconv.Itoa(rand.Intn(100000000))
i suggest using the crypto/rand package to generate a number here, assuming the
nonce is used cryptographically.

https://codereview.appspot.com/7228069/diff/1/oauth.go#newcode43
oauth.go:43: type _PLAINTEXTOAuthSigner struct {
the caps are rather jarring. why not plainTextOAuthSigner and lose the
underscores?

https://codereview.appspot.com/7228069/diff/1/oauth.go#newcode48
oauth.go:48: func NewPLAINTEXTOAuthSigner(token *OAuthToken, realm string)
(OAuthSigner, error) {
more conventional to return a pointer, *OAuthSigner.

https://codereview.appspot.com/7228069/diff/1/oauth.go#newcode68
oauth.go:68: authHeader := []string{}
var authHeader []string

is can be used instead and is slightly less noisy.

https://codereview.appspot.com/7228069/diff/1/templates/source.go
File templates/source.go (right):

https://codereview.appspot.com/7228069/diff/1/templates/source.go#newcode4
templates/source.go:4: package gomaasapi
what's this file for?

https://codereview.appspot.com/7228069/diff/1/testing.go
File testing.go (right):

https://codereview.appspot.com/7228069/diff/1/testing.go#newcode25
testing.go:25: handler := func(writer http.ResponseWriter, request
*http.Request) {
rather than use a closure, it might be clearer to use a type:

type serveOnce struct {
   mu sync.Mutex
   content string
   header http.Header
   requested bool
}
func (s *serveOnce) ServeHTTP(w http.ResponseWriter, request *http.Request) {
}

in general this is easier to debug too, because you can easily inspect the
fields in the struct.

note that you'll want a mutex because it's fine for ServeHTTP methods to be
called concurrently.

[jtv.canonical, 2013-02-05 11:25:19]

On 2013/02/01 10:43:41, TheMue wrote:

> - some missing comments,
> - sometimes non-go-typical identifiers and
> - would like more explicit oauth tests.

Thanks for looking this over.  One of the non-go-typical identifiers is
resource_uri, which looks that way to mimic what it represents.  I could change
it to Go style though.  Any other cases?


> Only major item is the JSONObject. Looks too complicated and heavy.

It provides an abstraction layer slightly above that of having the caller cast
their way around.  Which for instance means that we get to define (which we're
by no means done doing) what constitutes an error and what should be a panic. 
We can tighten this down more later, but our hope is to make this low-level
abstraction unnecessary and provide something closer to MAAS-native object types
instead.


> Also using strings as types instead of 
> 
> const (
>     StringType = iota
>     IntType
>     ...
> )

Yes, that would be worth changing.  We'll also want a stringer implementation
for it, I guess.


> Why isn't the direct usage of JSON serialization not usable here?

Because we needed to support full-featured MAAS objects contained inside JSON
objects or lists, without any static knowledge about what was inside those MAAS
objects apart from resource_uri.


> https://codereview.appspot.com/7228069/diff/1/client.go#newcode15
> client.go:15: type Client struct {
> Please add godoc documentation on all public types and functions.

I don't think we need to export this any more.  Roger mentioned something
called... gosym, I think, that would facilitate the renaming.


> https://codereview.appspot.com/7228069/diff/1/client.go#newcode24
> client.go:24: func (client Client) dispatchRequest(request *http.Request)
> ([]byte, error) {
> Here (c Client) is more usual.

Is it considered better for reading, or is it done only because it's easier to
write?  We're willing to do a bit more typing if that makes reading easier,
since legibility is very important to us — but if it's considered un-idiomatic
we can favour single-letter identifiers.

[jtv.canonical, 2013-02-05 11:49:47]

On 2013/02/01 10:43:41, TheMue wrote:

> https://codereview.appspot.com/7228069/diff/1/client.go#newcode36
> client.go:36: return body, errors.New("Error requesting the MAAS server: " +
> response.Status + ".")
> return nil, fmt.Errorf("gomaasapi: cannot dispatch request: %v",
> response.Status)
> 
> In case of an error no value is returned.

AFAICS this error isn't actually a failure to dispatch the request; it's a
successfully dispatched request receiving an undesirable response from the
server.  The problem is that in such a case, the body will generally contain
useful information about the error.

[jtv.canonical, 2013-02-05 12:08:10]

I addressed all of the trivial issues that TheMue raised, plus as many as I had
time for (perhaps slightly more) that rog brought up, in this branch:
https://code.launchpad.net/~jtv/gomaasapi/review-changes-1/+merge/146615

Raphaël is also planning to look into this, but hopefully this will take some of
the drudgery off his plate.

[fwereade, 2013-02-05 12:26:31]

+1s to just about everything rog and frank said, and a few comments on
JSONObject:

https://codereview.appspot.com/7228069/diff/1/jsonobject.go
File jsonobject.go (right):

https://codereview.appspot.com/7228069/diff/1/jsonobject.go#newcode32
jsonobject.go:32: GetString() (string, error)
On 2013/02/01 17:30:05, rog wrote:
> i don't really see what these methods have to offer over:
> 
> s, ok := someValue.(string)

The errors are quite helpful, I think. But I don't think JSONObject needs all
these Get methods when we could have a bunch of GetFoo(obj jsonObject) functions
-- I think it's be a lot cleaner. See below.

https://codereview.appspot.com/7228069/diff/1/jsonobject.go#newcode65
jsonobject.go:65: // Internal: turn a completely untyped json.Unmarshal result
into a
It's also conventional to start doc comments with the name of the thing being
commented on; it makes it neater to use the comment as a sentence in other
contexts. I know it's academic in this case, but worth bearing in mind in
general.

https://codereview.appspot.com/7228069/diff/1/jsonobject.go#newcode119
jsonobject.go:119: return errors.New(msg)
On 2013/02/01 17:30:05, rog wrote:
> fmt.Errorf("cannot convert %#v to %s", obj, wanted_type)
+1

https://codereview.appspot.com/7228069/diff/1/jsonobject.go#newcode159
jsonobject.go:159: func (obj jsonString) GetBool() (bool, error)                
{ return failBool(obj) }
This bit is awfully dense and repetitive. I think a few standalone Get* funcs
would work a bit better here, as mentioned above:

func (jsonString) Type() string  { return "string" }
func (jsonFloat64) Type() string { return "float64" }
...

func GetString(obj JSONObject) (string, error) {
    if obj.Type() == "string" {
        return string(obj), nil
    }
    return "", failConversion("string", obj)
}

func GetFloat64(obj JSONObject) (float64, error) {
    if obj.Type() == "float64" {
        return float64(obj), nil
    }
    return "", failConversion("float64", obj)
}

...

https://codereview.appspot.com/7228069/diff/1/maasobject.go
File maasobject.go (right):

https://codereview.appspot.com/7228069/diff/1/maasobject.go#newcode57
maasobject.go:57: panic("Cannot create jsonMAASObject object, no 'resource_uri'
key present in the given jsonMap.")
Conventional to panic with errors

[rog, 2013-02-05 13:49:57]

https://codereview.appspot.com/7228069/diff/1/jsonobject.go
File jsonobject.go (right):

https://codereview.appspot.com/7228069/diff/1/jsonobject.go#newcode159
jsonobject.go:159: func (obj jsonString) GetBool() (bool, error)                
{ return failBool(obj) }
On 2013/02/05 12:26:31, fwereade wrote:
> This bit is awfully dense and repetitive. I think a few standalone Get* funcs
> would work a bit better here, as mentioned above:
> 
> func (jsonString) Type() string  { return "string" }
> func (jsonFloat64) Type() string { return "float64" }
> ...
> 
> func GetString(obj JSONObject) (string, error) {
>     if obj.Type() == "string" {
>         return string(obj), nil
>     }
>     return "", failConversion("string", obj)
> }
> 
> func GetFloat64(obj JSONObject) (float64, error) {
>     if obj.Type() == "float64" {
>         return float64(obj), nil
>     }
>     return "", failConversion("float64", obj)
> }
> 
> ...

if that's the case, why bother with the JSONObject type at all?
why not:

func GetString(obj interface{}) (string, error) {
    if s, ok := obj.(string); ok {
        return s, nil
    }
    return "", failConversion("string", obj)
}

or even a single function:

// Get sets the value of into (which must be a pointer
// to one of float64, string, map[string]interface{},
// bool or MaasObject) to the value of jsonObj.
func Get(jsonObj interface{}, into interface{}) error {
    var ok bool
    switch into := into.(type) {
    case *string:
        *into, ok = jsonObj.(string)
    case *float64:
        *into, ok = jsonObj.(float64)
    etc
    }
    if !ok {
        return failConversion(reflect.TypeOf(into).Elem().String(), jsonObj)
    }
    return nil
}

but all this seems like it's trying to do something
similar to launchpad.net/juju-core/schema without
the nice generality that that package provides.

[TheMue, 2013-02-05 14:04:27]

On 2013/02/05 13:49:57, rog wrote:

> but all this seems like it's trying to do something
> similar to launchpad.net/juju-core/schema without
> the nice generality that that package provides.

+1

[rvb, 2013-02-05 16:28:19]

Thanks a lot for the reviews, another batch of improvements:
https://code.launchpad.net/~rvb/gomaasapi/review-changes-2/+merge/146627

[jtv.canonical, 2013-02-06 04:01:48]

And another:
https://code.launchpad.net/~jtv/gomaasapi/review-changes-3/+merge/146768

[rog, 2013-02-06 07:53:42]

it would be fantastic if these changes could be proposed
with lbox -cr. if that's not feasible, i'll review in the old fashioned
way, but i'd prefer to be able to comment inline.


On 6 February 2013 04:01,  <jtv.canonical@gmail.com> wrote:
> And another:
> https://code.launchpad.net/~jtv/gomaasapi/review-changes-3/+merge/146768
>
> https://codereview.appspot.com/7228069/

[gz, 2013-02-06 16:59:23]

Please take a look.

[TheMue, 2013-02-06 18:53:34]

A lot of improvements, some points still not covered. Additionally I'm still
unsure about the JSON stuff and would like to see an approach similar to
launchpad.net/juju-core/schema or with more generic functions like Roger
described yesterday.

https://codereview.appspot.com/7228069/diff/16001/client.go
File client.go (right):

https://codereview.appspot.com/7228069/diff/16001/client.go#newcode22
client.go:22: func (client Client) dispatchRequest(request *http.Request)
([]byte, error) {
Still prefer (c *Client) as it is a common Go style.

https://codereview.appspot.com/7228069/diff/16001/example/live_example.go
File example/live_example.go (right):

https://codereview.appspot.com/7228069/diff/16001/example/live_example.go#new...
example/live_example.go:24: if err != nil {
checkError() can be used here too.

https://codereview.appspot.com/7228069/diff/16001/example/live_example.go#new...
example/live_example.go:29: if err != nil {
Same here.

https://codereview.appspot.com/7228069/diff/16001/jsonobject.go
File jsonobject.go (right):

https://codereview.appspot.com/7228069/diff/16001/jsonobject.go#newcode65
jsonobject.go:65: // maasify is internal.  It turns a completely untyped
json.Unmarshal result
No need to tell it's internal, only describe what it does.