launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Review: Approve

The base URL being passed down to MAASObject is surprising to me.  I would have expected the base URL to be encapsulated in the Client so that the client's Get()/Post()/Put()/Delete() are inherently relative to the base URL.  (Or if we ever get absolute resource URIs, it'd be up to the Client to reconcile those with its base URI).  That is also what we did in the python API wrapper.


Miscellaneous notes:

1. [In client.go] Could you document modification()?  There may even be some better name for it, which better fits the notion that Delete() isn't a modification().

2. [In client.go] Why accept an empty "operation" string?  Given that this is the MAAS API, that looks more like an error to me.

3. [In client_test.go] TestClientDeleteSendsRequest does not verify that the right request is made, only that Delete() returns no error when you expect no error.  You can't test the request as easily as you can with the other methods, but you can verify that Delete() does return an error when the request is to the wrong URL.  In Python we would test a good proportion of such error conditions, but in Go it looks like the overhead of testing is so high that we have to pay attention to what's worth testing and what isn't.  :(

4. [In live_example.go] I think you're making a sensible choice by just panicking if something goes wrong.  It helps keep the code brief.  But maybe this deserves a top comment, so that somebody else doesn't think it's by accident.  If they helpfully add error checking that's fine, but it's not so fine if they base their own programs on this code, or file bug reports about the missing error checks.

5. [In live_example.go] Is there a clear warning that this example messes with the data on the server?  If deletes the node it creates once it's done, but that's not much help if for whatever reason the program does not complete.

6. [In maasobject.go] The name SubObject() confuses me.  It looks like two words, in which case I would really expect the first word to be a verb.  How about GetSubObject()?

7. [In maasobject.go] Does Post() sometimes return something that is not a MAASObject?  I gave it a quick look when I wrote the object-handling code, but what I found suggested that a non-method POST conventionally returns the object you just posted.  (A POST to a method call of course may return lists and other JSON).

8. [In maasobject.go] When panicking in response to an error, it'd be nice to include the error message.

9. [In maasobject_test.go] SubObject() is untested.

10. [In server.go] NewServer() is undocumented, and its purpose is not very clear — it looks like a constructor for a nonexistent Server class.  I would describe the function as "presenting an instance of the MAAS API as a MAASObject," but that doesn't use the word "server" at all.  In fact I wouldn't use that word here, because the MAAS terminology no longer includes it.  "Server" could refer to a node, a cluster controller, a region controller, or some other server system with roughly equal validity.

11. [In testing.go] Not something you did, in fact you're already improving this line, but if you have time: "404 page not found" is a horrible error message.  It indicates that the 404 page was not found, which is probably not what was meant!  Some punctuation between the error number and the error text would be enough to resolve this.  (Actuall http.StatusNotFound is just as bad, but there's little we can do about that.)


Jeroen
-- 
https://code.launchpad.net/~rvb/maas/gomaasapi-junction/+merge/145001
Your team MAAS Maintainers is subscribed to branch lp:~maas-maintainers/maas/gomaasapi.