Story #2826

Create a Go SDK for arvados

Added by Misha Zatsman over 6 years ago. Updated over 6 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Start date:
05/14/2014
Due date:
% Done:

100%

Estimated time:
(Total: 4.50 h)
Story points:
1.0

Description


Subtasks

Task #2994: Generalize keep client code that talks to API serverResolvedPeter Amstutz

Task #3002: Review 2826-simple-go-sdkResolvedTim Pierce

Associated revisions

Revision d161e228 (diff)
Added by Peter Amstutz over 6 years ago

Merge branch '2826-simple-go-sdk' closes #2826

History

#1 Updated by Tom Clegg over 6 years ago

  • Target version set to 2014-05-28 Pipeline Factory

#2 Updated by Tom Clegg over 6 years ago

  • Story points changed from 21.0 to 5.0

#3 Updated by Tom Clegg over 6 years ago

  • Description updated (diff)

#4 Updated by Tom Clegg over 6 years ago

  • Target version changed from 2014-05-28 Pipeline Factory to 2014-06-17 Curating and Crunch

#5 Updated by Peter Amstutz over 6 years ago

  • Assigned To changed from Misha Zatsman to Peter Amstutz
  • Story points changed from 5.0 to 1.0

Generalize existing keep client code that talks to API server

#6 Updated by Tim Pierce over 6 years ago

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

#7 Updated by Peter Amstutz over 6 years ago

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

#8 Updated by Tim Pierce over 6 years ago

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.

#9 Updated by Anonymous over 6 years ago

  • Status changed from New to Resolved

Applied in changeset arvados|commit:d161e2280a03b4a1c9675f5ed1946310a9942acd.

Also available in: Atom PDF