Arvados

This is nice! I like the refactoring a lot and it makes it easier to write a high-level Arvados client in Go. A couple of functional requests but mostly style.

For what it's worth, I'm still confused by the use of pointers for KeepClient and ArvadosClient instances. Sometimes a client instance is a pointer and sometimes it isn't, and there are places in the code that explicitly use & and * operators to handle it appropriately. I would strongly prefer to follow the convention that MakeFooClient always returns a pointer to a client struct, and define all method receivers to use pointers, so there's no lexical ambiguity in how to use the client object in code.

I suspect we're not going to agree, so I won't block the review, but I'm concerned that the code will be harder to maintain going forward, so I hope we can at least keep arguing about it :-)

Comments @ 906022fee:

• arvados.org/keepclient/support.go
  • DiscoverKeepServers(): "defer" comment is obsolete
  • uploadToKeepServer(): It would be more appropriate for this method to use http.DefaultClient.Do than this.Arvados.Client.Do, since it's not issuing a request to the Arvados API server and the Arvados.Client object has been configured to match the API server's TLS policy, not the Keep server's.
• arvados.org/sdk/sdk.go
  • ArvadosClient should document its fields
  • sdk.MakeArvadosClient(): How about we give arguments for ApiHost, ApiToken and ApiHostInsecure, and let the caller decide how to generate them? For convenience we can add a DefaultArvadosClient() method that creates a client from the environment variables. That gives client apps more flexibility in creating an Arvados client and eliminates their having to change environment vars just to create one. If you don't object to this proposal but it's too much hassle right now then a TODO would be fine.
  • Can we rename the "Dict" type something a little more descriptive like "UrlParamMap" or "ExtraUrlParams"?
  • TODO: tests for other arv client requests, tests for errors returned by bad requests

• Removed dangling "defer" comment
• Added a separate http Client field to KeepClient instead of using the ArvadosClient http Client object
• Added comments to ArvadosClient
• Fixed ArvadosClient methods take a copy instead of a struct to denote that they are free from side effects
• MakeArvadosClient() pretty much doesn't do anything except read from the environment; if you want flexibility it's easy enough to just initialize the structure directly.
• The only purpose of the "Dict" type was to be a shorthand because writing out 'map[string]interface{}' when building a nested structure is annoying, so giving it a longer name would kind of defeat that goal. A better approach would be to accept a interface{} and use reflection on the struct fields, but that seemed excessive for a first pass implementation. (I can't just use json marshaling directly the way I'm using json unmarshaling because it has to be URL form encoded...)
• There can always be more tests, but I think the current testing is adequate, keep in mind that I ported keepclient/keepproxy to use the new SDK so the SDK is being indirectly tested by the keepclient and keepproxy tests as well.

Peter Amstutz wrote:

• There can always be more tests, but I think the current testing is adequate, keep in mind that I ported keepclient/keepproxy to use the new SDK so the SDK is being indirectly tested by the keepclient and keepproxy tests as well.

I agree that the current level of testing is adequate -- that's why I'm only asking for a TODO. :-)

Other than that, LGTM.