launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

> 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.

Thanks for the review.  Let's talk about this and see what we can do about it.

I've fixed a bunch of stuff and left some others so that we can fix them in follow-ups branches:

> 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().

I've renamed it "nonIdempotentRequest" and added a comment.


> 
> 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.

Done.

> 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.

Done.

> 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()?

Good point, done.

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

Added a test.

> 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.

Good point, done (NewServer => NewMAAS).

> 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.)

Not yet done:

> 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.  :(

> 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.

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