Issue 6782112: Initial glance client (Closed)
|
Created:
11 years, 5 months ago by wallyworld Modified:
11 years, 4 months ago Reviewers:
mp+136081, rog Visibility:
Public. |
DescriptionInitial glance client
This branch implements an initial glance client. Only a few basic APIs are implemented since I'm not sure exactly what we'll need to provide. New APIs can easily be added as required.
I've also renamed a few interfaces.
https://code.launchpad.net/~wallyworld/goose/initial-glance-module/+merge/136081
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 25
Patch Set 2 : Initial glance client #
Total comments: 9
Patch Set 3 : Initial glance client #
Total comments: 45
Patch Set 4 : Initial glance client #Patch Set 5 : Initial glance client #
Total comments: 6
Patch Set 6 : Initial glance client #
MessagesTotal messages: 19
LGTM overall, some small tweaks. https://codereview.appspot.com/6782112/diff/1/[revision%20details] File [revision details] (right): https://codereview.appspot.com/6782112/diff/1/[revision%20details]#newcode2 [revision details]:2: New revision: ian.booth@canonical.com-20121126022608-aozynrv5vamj3ngi What generated this file? https://codereview.appspot.com/6782112/diff/1/glance/glance.go File glance/glance.go (right): https://codereview.appspot.com/6782112/diff/1/glance/glance.go#newcode25 glance/glance.go:25: client client.Client Naming here. I thought we moved away from "OpenStackNovaClient" so we might want to name the glance version of it something different. Maybe just GlanceClient. https://codereview.appspot.com/6782112/diff/1/glance/glance.go#newcode52 glance/glance.go:52: "failed to get list of images") Why is API_IMAGES getting sent to "compute"? I'm pretty sure: { "endpoints_links": [], "endpoints": [ { "adminURL": "http://10.55.58.1:9292/v1", "region": "lcy01", "publicURL": "http://10.55.58.1:9292/v1", "internalURL": "http://10.55.58.1:9292/v1", "id": "0951f70d0543490c84bb4574df0c3f1a" }, { "adminURL": "http://10.55.62.1:9292/v1", "region": "lcy02", "publicURL": "http://10.55.62.1:9292/v1", "internalURL": "http://10.55.62.1:9292/v1", "id": "904e96b7efd84065a94886547c080a10" } ], "type": "image", "name": "glance" }, I'm pretty sure you want to supply "image" in all of these. Vs: "serviceCatalog": [ { "endpoints_links": [], "endpoints": [ { "adminURL": "http://10.55.58.1:8774/v2/cf05a07a30aa45369a114e28a06530e4", "region": "lcy01", "publicURL": "https://nova-lcy01.canonistack.canonical.com/v2/cf05a07a30aa45369a114e28a06530e4", "internalURL": "http://10.55.58.1:8774/v2/cf05a07a30aa45369a114e28a06530e4", "id": "72b6fb73834242efb06b91d553000557" }, { "adminURL": "http://10.55.62.1:8774/v2/cf05a07a30aa45369a114e28a06530e4", "region": "lcy02", "publicURL": "https://nova-lcy02.canonistack.canonical.com/v2/cf05a07a30aa45369a114e28a06530e4", "internalURL": "http://10.55.62.1:8774/v2/cf05a07a30aa45369a114e28a06530e4", "id": "0227c5f6504448f28660d1718d5bcc15" } ], "type": "compute", "name": "nova" }, I'm a bit surprised that it works if you send the glance requests to the compute node, but I'm pretty sure we don't want to do that long term. https://codereview.appspot.com/6782112/diff/1/glance/glance.go#newcode95 glance/glance.go:95: url := fmt.Sprintf("%s/%s", OS_API_IMAGES, imageId) I'm guessing we shouldn't call it URL if it is only a URL fragment. Maybe 'path' or url_fragment, or ...? I'm not sure the best thing, but http://www.w3.org/Addressing/URL/4_3_Partial.html suggests 'partial_uri', so maybe "partial_url"? I also feel like we're seeing a fair amount of the pattern of "this api location + this fragment", so maybe we want to add a parameter to SendRequest and have it do the fmt.Sprintf ? https://codereview.appspot.com/6782112/diff/1/glance/glance_test.go File glance/glance_test.go (right): https://codereview.appspot.com/6782112/diff/1/glance/glance_test.go#newcode36 glance/glance_test.go:36: } This is really looking like something that belongs in identity as a "CompleteCredentialsFromEnv" or something along those lines. We shouldn't have to reproduce this in each test suite. https://codereview.appspot.com/6782112/diff/1/glance/glance_test.go#newcode45 glance/glance_test.go:45: s.imageId = "ceee61e9-c7a5-4e51-ae61-6770e359e341" Is this something that is owned by a specific user? Or is this public or? maybe expand the comment to understand where that image Id came from, and how we could generate one in the future? https://codereview.appspot.com/6782112/diff/1/glance/glance_test.go#newcode52 glance/glance_test.go:52: c.Assert(err, IsNil) We probably want to assert that the len(images) is not 0. Otherwise all the follow up asserts will never trigger. https://codereview.appspot.com/6782112/diff/1/glance/glance_test.go#newcode68 glance/glance_test.go:68: } same thing here https://codereview.appspot.com/6782112/diff/1/glance/glance_test.go#newcode86 glance/glance_test.go:86: ir, err := s.glance.GetImageDetail(s.imageId) Since this is the only location that uses this imageId, it feels like it should be a variable only for this test, rather than a Suite attribute. Is the idea to have more tests using it? https://codereview.appspot.com/6782112/diff/1/nova/nova.go File nova/nova.go (left): https://codereview.appspot.com/6782112/diff/1/nova/nova.go#oldcode22 nova/nova.go:22: type NovaProvider interface { It would probably be easier to review in the future if you split out the rename of classes from the implementation of glance. All of the specific name changes seem fine to me, just easier to review as separate actions.
Sign in to reply to this message.
looks reasonable, except i am concerned about the replication of very similar APIs with small variations. surely there's something we can do about that? https://codereview.appspot.com/6782112/diff/1/glance/glance.go File glance/glance.go (right): https://codereview.appspot.com/6782112/diff/1/glance/glance.go#newcode11 glance/glance.go:11: OS_API_IMAGES = "/images" unexport? https://codereview.appspot.com/6782112/diff/1/glance/glance.go#newcode15 glance/glance.go:15: // Provide access to the OpenStack Glance service. // GlanceClient provides... is there a reason this is an interface? can't NewGlanceClient just return a *OpenStackGlanceClient? https://codereview.appspot.com/6782112/diff/1/glance/glance.go#newcode25 glance/glance.go:25: client client.Client On 2012/11/26 13:07:56, john.meinel wrote: > Naming here. I thought we moved away from "OpenStackNovaClient" so we might want > to name the glance version of it something different. > > Maybe just GlanceClient. glance.Client reads well to me https://codereview.appspot.com/6782112/diff/1/glance/glance.go#newcode33 glance/glance.go:33: type Link struct { these types look very similar (dentical?) to the types in the other client packages. perhaps we could define them in another package (goose itself?) and use them throughout. https://codereview.appspot.com/6782112/diff/1/glance/glance.go#newcode79 glance/glance.go:79: func (n *OpenStackGlanceClient) ListImagesDetail() (images []ImageDetail, err error) { i appreciate that glance has "images" and goose/client has "flavors", but ISTM that consistency in naming might be a good thing as they seem to be representing a very similar thing. assuming there *is* some commonality between them, how about defining a common interface between them, exposing the lowest-common-denominator information, but providing a way to get more. for instance (in some other package): type Image struct { Id string Name string Links []Link } type ImageDetail interface { Id() string Name() string // ... and whatever else is common currency } type Client interface { Images() ([]Image, error) ImageDetails(imageId string) ([]ImageDetail, error) } https://codereview.appspot.com/6782112/diff/1/glance/glance_test.go File glance/glance_test.go (right): https://codereview.appspot.com/6782112/diff/1/glance/glance_test.go#newcode36 glance/glance_test.go:36: } On 2012/11/26 13:07:56, john.meinel wrote: > This is really looking like something that belongs in identity as a > "CompleteCredentialsFromEnv" or something along those lines. We shouldn't have > to reproduce this in each test suite. +1 https://codereview.appspot.com/6782112/diff/1/nova/nova.go File nova/nova.go (right): https://codereview.appspot.com/6782112/diff/1/nova/nova.go#newcode23 nova/nova.go:23: ListFlavors() (flavors []Entity, err error) i wonder about these names, and whether leaving out the "List" and "Get" prefixes might make for a nicer feel. e.g. type Nova interface { Flavors() ([]Entity, error) FlavorDetails() ([]FlavorDetail, error) Servers() ([]Entity, error) ServerDetails() ([]ServerDetail, error) Server(serverId string) (ServerDetail, error) etc } apart from that, why do we need this interface type?
Sign in to reply to this message.
So, juju doesn't actually require any of the glance apis, but adding it to goose is reasonable anyway. Also brings up an issue we're going to run into when juju adds storage support and starts needing the volume apis... which is where to find api calls that used to live in nova before being split out to a new project. Ideally we'd just look at the endpoints keystone gives us, but as below with canonistack, in practice it's not nearly that neat. https://codereview.appspot.com/6782112/diff/1/[revision%20details] File [revision details] (right): https://codereview.appspot.com/6782112/diff/1/[revision%20details]#newcode2 [revision details]:2: New revision: ian.booth@canonical.com-20121126022608-aozynrv5vamj3ngi On 2012/11/26 13:07:56, john.meinel wrote: > What generated this file? It's an lbox artefact, just used to track what rev it's based on I think. https://codereview.appspot.com/6782112/diff/1/glance/glance.go File glance/glance.go (right): https://codereview.appspot.com/6782112/diff/1/glance/glance.go#newcode52 glance/glance.go:52: "failed to get list of images") On 2012/11/26 13:07:56, john.meinel wrote: > Why is API_IMAGES getting sent to "compute"? So, here's the fun: > { ... > "publicURL": "http://10.55.58.1:9292/v1", ... > "type": "image", > "name": "glance" > }, vs. > { > "publicURL": > "https://nova-lcy01.canonistack.canonical.com/v2/cf05a07a30aa45369a114e28a06530e4", ... > "type": "compute", > "name": "nova" > }, It has been a few releases since glance was split out from nova, but the api is still exposed through nova (for compat), and as you can see from the above canonistack does not actually put the glance api endpoint anywhere accessible. Apart from parsing the publicURL and going "hey, this is a 10. address" there's no sane way of telling that given the way they have things configured, unfortunately. Swift was like this for a long time before IS gave it an endpoint with dns that resolved to something outside the local network. https://codereview.appspot.com/6782112/diff/1/glance/glance_test.go File glance/glance_test.go (right): https://codereview.appspot.com/6782112/diff/1/glance/glance_test.go#newcode45 glance/glance_test.go:45: s.imageId = "ceee61e9-c7a5-4e51-ae61-6770e359e341" Even for live tests, we don't want to be hardcoding things like ids. If we want a specific id, we should use listids to pick one, and skip if none are available.
Sign in to reply to this message.
https://codereview.appspot.com/6782112/diff/1/glance/glance.go File glance/glance.go (right): https://codereview.appspot.com/6782112/diff/1/glance/glance.go#newcode15 glance/glance.go:15: // Provide access to the OpenStack Glance service. On 2012/11/26 14:32:38, rog wrote: > // GlanceClient provides... > > is there a reason this is an interface? can't NewGlanceClient just return a > *OpenStackGlanceClient? Years of ingrained programming "best practice" habits result in an interface being defined for major components, and the interface being used instead of the component impl for references and return types etc. Perhaps that's not the Go way? Because we are writing a double/mock for the core services, the idea was to have the real impl and mock implement the above interface, and then the same tests could be run against the real impl and mock, simply by referring to the interface. https://codereview.appspot.com/6782112/diff/1/glance/glance.go#newcode25 glance/glance.go:25: client client.Client On 2012/11/26 13:07:56, john.meinel wrote: > Naming here. I thought we moved away from "OpenStackNovaClient" so we might want > to name the glance version of it something different. > > Maybe just GlanceClient. Yes, I missed this one. https://codereview.appspot.com/6782112/diff/1/glance/glance.go#newcode79 glance/glance.go:79: func (n *OpenStackGlanceClient) ListImagesDetail() (images []ImageDetail, err error) { On 2012/11/26 14:32:38, rog wrote: > i appreciate that glance has "images" and goose/client has "flavors", but ISTM > that consistency in naming might be a good thing as they seem to be representing > a very similar thing. > > assuming there *is* some commonality between them, how about defining a common > interface between them, exposing the lowest-common-denominator information, but > providing a way to get more. > > for instance (in some other package): > > type Image struct { > Id string > Name string > Links []Link > } > > type ImageDetail interface { > Id() string > Name() string > // ... and whatever else is common currency > } > > type Client interface { > Images() ([]Image, error) > ImageDetails(imageId string) ([]ImageDetail, error) > } > There's still work to be done to properly define and manage the definition of the data model exchanged between client and server. eg different API versions, different OpenStack releases, different vendor flavours. So for now it's just hard coded to whatever works against Canonistack just to get something going and it will be properly dealt with in time. https://codereview.appspot.com/6782112/diff/1/glance/glance_test.go File glance/glance_test.go (right): https://codereview.appspot.com/6782112/diff/1/glance/glance_test.go#newcode36 glance/glance_test.go:36: } On 2012/11/26 13:07:56, john.meinel wrote: > This is really looking like something that belongs in identity as a > "CompleteCredentialsFromEnv" or something along those lines. We shouldn't have > to reproduce this in each test suite. Agreed, I just initially wanted to have something written and now that there's a few instances of this code we can swing back around and do some sensible refactoring. https://codereview.appspot.com/6782112/diff/1/glance/glance_test.go#newcode86 glance/glance_test.go:86: ir, err := s.glance.GetImageDetail(s.imageId) On 2012/11/26 13:07:56, john.meinel wrote: > Since this is the only location that uses this imageId, it feels like it should > be a variable only for this test, rather than a Suite attribute. Is the idea to > have more tests using it? Yes, I followed the pattern already established for the nova tests. I only initially implemented a few of the glance apis just to get something done. I'm not sure exactly what we'll need moving forward.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
On 26 November 2012 23:04, <ian.booth@canonical.com> wrote: > Reviewers: mp+136081_code.launchpad.net, john.meinel, rog, gz, > > > > https://codereview.appspot.com/6782112/diff/1/glance/glance.go > File glance/glance.go (right): > > https://codereview.appspot.com/6782112/diff/1/glance/glance.go#newcode15 > glance/glance.go:15: // Provide access to the OpenStack Glance service. > On 2012/11/26 14:32:38, rog wrote: >> >> // GlanceClient provides... > > >> is there a reason this is an interface? can't NewGlanceClient just > > return a >> >> *OpenStackGlanceClient? > > > Years of ingrained programming "best practice" habits result in an > interface being defined for major components, and the interface being > used instead of the component impl for references and return types etc. > Perhaps that's not the Go way? Because we are writing a double/mock for > the core services, the idea was to have the real impl and mock implement > the above interface, and then the same tests could be run against the > real impl and mock, simply by referring to the interface. Usually it's not the Go way, because when you test some logic, you can decide in that place to use an interface or not, and define the tests and interface for that occasion, rather than defining the interface up front. If you're implementing mock http servers for the openstack stuff, I think that mocking at that level should be sufficient.
Sign in to reply to this message.
https://codereview.appspot.com/6782112/diff/1009/glance/glance.go File glance/glance.go (right): https://codereview.appspot.com/6782112/diff/1009/glance/glance.go#newcode24 glance/glance.go:24: type GlanceClient struct { i might have said it before, but i still think it's true - the "Glance" in this name is redundant. as it is above, it will always be referred to as "glance.GlanceClient". "glance.Client" reads much more cleanly IMHO. "glance.NewClient" too. https://codereview.appspot.com/6782112/diff/1009/glance/glance.go#newcode46 glance/glance.go:46: the blank line at the start of each function seems a little awkward to me, but if you're going to standardise on it, you might as well put one at the start of all functions (e.g. NewGlanceClient above) https://codereview.appspot.com/6782112/diff/1009/glance/glance.go#newcode98 glance/glance.go:98: "failed to get details for imageId=%s", imageId) it might be worth explicitly returning ImageDetail{}, nil on error. i'm not sure if it's guaranteed that a json unmarshal cannot return an error half-way through filling out a struct. https://codereview.appspot.com/6782112/diff/1009/identity/identity.go File identity/identity.go (right): https://codereview.appspot.com/6782112/diff/1009/identity/identity.go#newcode73 identity/identity.go:73: panic(fmt.Sprintf("required environment variable not set for credentials attribute: %s", t.Field(i).Name)) this seems wrong in a non-test context. much better to return an error. https://codereview.appspot.com/6782112/diff/1009/nova/nova.go File nova/nova.go (right): https://codereview.appspot.com/6782112/diff/1009/nova/nova.go#newcode66 nova/nova.go:66: type NovaClient struct { as with Glance, so with Nova. "nova.Client" is just nicer (and more idiomatically Go-like) than nova.NovaClient.
Sign in to reply to this message.
On 2012/11/27 13:09:03, rog wrote: > On 26 November 2012 23:04, <mailto:ian.booth@canonical.com> wrote: > > Reviewers: http://mp+136081_code.launchpad.net, john.meinel, rog, gz, > > > > > > > > https://codereview.appspot.com/6782112/diff/1/glance/glance.go > > File glance/glance.go (right): > > > > https://codereview.appspot.com/6782112/diff/1/glance/glance.go#newcode15 > > glance/glance.go:15: // Provide access to the OpenStack Glance service. > > On 2012/11/26 14:32:38, rog wrote: > >> > >> // GlanceClient provides... > > > > > >> is there a reason this is an interface? can't NewGlanceClient just > > > > return a > >> > >> *OpenStackGlanceClient? > > > > > > Years of ingrained programming "best practice" habits result in an > > interface being defined for major components, and the interface being > > used instead of the component impl for references and return types etc. > > Perhaps that's not the Go way? Because we are writing a double/mock for > > the core services, the idea was to have the real impl and mock implement > > the above interface, and then the same tests could be run against the > > real impl and mock, simply by referring to the interface. > > Usually it's not the Go way, because when you test some logic, you > can decide in that place to use an interface or not, and define > the tests and interface for that occasion, rather than defining the interface > up front. > > If you're implementing mock http servers for the openstack > stuff, I think that mocking at that level should be sufficient. The various interfaces, as defined, are used in the tests. The test suites declare an attribute of the interface type to hold a reference to the client impl under test.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6782112/diff/1009/glance/glance.go File glance/glance.go (right): https://codereview.appspot.com/6782112/diff/1009/glance/glance.go#newcode24 glance/glance.go:24: type GlanceClient struct { On 2012/11/27 14:56:29, rog wrote: > i might have said it before, but i still think it's true - the "Glance" in this > name is redundant. as it is above, it will always be referred to as > "glance.GlanceClient". > > "glance.Client" reads much more cleanly IMHO. > "glance.NewClient" too. Sure. I'll also change the NovaClient and SwiftClient struct names accordingly. I'm still getting used to the Go way of doing things. https://codereview.appspot.com/6782112/diff/1009/glance/glance.go#newcode46 glance/glance.go:46: On 2012/11/27 14:56:29, rog wrote: > the blank line at the start of each function seems a little awkward to me, but > if you're going to standardise on it, you might as well put one at the start of > all functions (e.g. NewGlanceClient above) I don't think that's a standard anyone has conscientiously decided to follow? I may have seen some places with the blank line and just cargo culted that. I'll remove the blank lines. https://codereview.appspot.com/6782112/diff/1009/glance/glance.go#newcode98 glance/glance.go:98: "failed to get details for imageId=%s", imageId) On 2012/11/27 14:56:29, rog wrote: > it might be worth explicitly returning ImageDetail{}, nil on error. i'm not sure > if it's guaranteed that a json unmarshal cannot return an error half-way through > filling out a struct. Yes, I'm not sure either. For now though, I'd like to leave this as is since all the other code follows the same pattern and I think if we are to change it, it should be done together is a separate clean up branch. https://codereview.appspot.com/6782112/diff/1009/identity/identity.go File identity/identity.go (right): https://codereview.appspot.com/6782112/diff/1009/identity/identity.go#newcode73 identity/identity.go:73: panic(fmt.Sprintf("required environment variable not set for credentials attribute: %s", t.Field(i).Name)) On 2012/11/27 14:56:29, rog wrote: > this seems wrong in a non-test context. > much better to return an error. Ah, yes good pickup. The code was copied from some tests where it was duplicated. I'll change to an error.
Sign in to reply to this message.
Thanks Ian. Coming along nicely. https://codereview.appspot.com/6782112/diff/8002/glance/glance.go File glance/glance.go (right): https://codereview.appspot.com/6782112/diff/8002/glance/glance.go#newcode16 glance/glance.go:16: type Glance interface { Tradition dictates that Public symbols are commented, either on the interface definition, or on the implementation. https://codereview.appspot.com/6782112/diff/8002/glance/glance.go#newcode45 glance/glance.go:45: remove space https://codereview.appspot.com/6782112/diff/8002/glance/glance.go#newcode88 glance/glance.go:88: func (c *Client) GetImageDetail(imageId string) (ImageDetail, error) { Can you please make the use of named error returns consistent. ie, add them to this method, or remove them from the previous methods. My preference is not to use them except when truly needed. https://codereview.appspot.com/6782112/diff/8002/glance/glance_test.go File glance/glance_test.go (right): https://codereview.appspot.com/6782112/diff/8002/glance/glance_test.go#newcode21 glance/glance_test.go:21: if !*live { nice https://codereview.appspot.com/6782112/diff/8002/glance/glance_test.go#newcode27 glance/glance_test.go:27: c.Fatalf("Error setting up test suite: %s", err.Error()) c.Fatalf("Error...: %v", err) errors know how to print themselves with %v, however in this case c.Assert(err, NotNil) is all you need. https://codereview.appspot.com/6782112/diff/8002/glance/glance_test.go#newcode42 glance/glance_test.go:42: c.Assert(err, IsNil) c.Assert(len(images), NotEqual, 0) maybe https://codereview.appspot.com/6782112/diff/8002/glance/glance_test.go#newcode43 glance/glance_test.go:43: if len(images) < 1 { c.Assert(images, Not(HasLen), 0) https://codereview.appspot.com/6782112/diff/8002/glance/glance_test.go#newcode59 glance/glance_test.go:59: if len(images) < 1 { ditto https://codereview.appspot.com/6782112/diff/8002/nova/nova.go File nova/nova.go (right): https://codereview.appspot.com/6782112/diff/8002/nova/nova.go#newcode22 nova/nova.go:22: type Nova interface { Please provide a brief comment on each interface method. https://codereview.appspot.com/6782112/diff/8002/nova/nova.go#newcode65 nova/nova.go:65: // Client represents a Nova client. // http://linktosomespec https://codereview.appspot.com/6782112/diff/8002/nova/nova.go#newcode66 nova/nova.go:66: type Client struct { But I think this might be a private type. https://codereview.appspot.com/6782112/diff/8002/nova/nova.go#newcode69 nova/nova.go:69: // NewClient returns a new Nova client https://codereview.appspot.com/6782112/diff/8002/nova/nova.go#newcode153 nova/nova.go:153: func (c *Client) GetServer(serverId string) (ServerDetail, error) { Same comments with respect to named vs unnamed return values. https://codereview.appspot.com/6782112/diff/8002/swift/swift.go File swift/swift.go (right): https://codereview.appspot.com/6782112/diff/8002/swift/swift.go#newcode10 swift/swift.go:10: // Provide access to the OpenStack Object Storage service. // Swift provides access to ... https://codereview.appspot.com/6782112/diff/8002/swift/swift.go#newcode11 swift/swift.go:11: type Swift interface { Comment interface methods please https://codereview.appspot.com/6782112/diff/8002/swift/swift.go#newcode25 swift/swift.go:25: type Client struct { I think this type should be private https://codereview.appspot.com/6782112/diff/8002/swift/swift.go#newcode28 swift/swift.go:28: // NewClient returns a new Swift client. https://codereview.appspot.com/6782112/diff/8002/swift/swift.go#newcode55 swift/swift.go:55: path := fmt.Sprintf("/%s/%s", containerName, objectName) There are lots of cases of Sprint'ing paths, some are absolute, some are relative, have you considered using path.Join to make this more regular ? https://codereview.appspot.com/6782112/diff/8002/swift/swift_test.go File swift/swift_test.go (right): https://codereview.appspot.com/6782112/diff/8002/swift/swift_test.go#newcode25 swift/swift_test.go:25: cred, err := identity.CompleteCredentialsFromEnv() c.Assert(err, NotNil) is usually good enough. https://codereview.appspot.com/6782112/diff/8002/swift/swift_test.go#newcode30 swift/swift_test.go:30: err = client.Authenticate() c.Assert(err, NotNIl)
Sign in to reply to this message.
>> Usually it's not the Go way, because when you test some logic, you >> can decide in that place to use an interface or not, and define >> the tests and interface for that occasion, rather than defining the > > interface >> >> up front. > > >> If you're implementing mock http servers for the openstack >> stuff, I think that mocking at that level should be sufficient. > > > The various interfaces, as defined, are used in the tests. The test > suites declare an attribute of the interface type to hold a reference to > the client impl under test. I see that. However, is there any particular reason you can't declare the interface in the tests rather than muddying the public API? Although I'm not sure that even that is useful - I can't see that using an interface in the tests rather than the concrete type is gaining anything. AFAICS your current test-double approach is mocking the http server rather than these interfaces. I think you could remove the interface definitions, use the concrete type throughout, and everything would work just fine.
Sign in to reply to this message.
On 2012/11/28 09:38:20, rog wrote: > >> Usually it's not the Go way, because when you test some logic, you > >> can decide in that place to use an interface or not, and define > >> the tests and interface for that occasion, rather than defining the > > > > interface > >> > >> up front. > > > > > >> If you're implementing mock http servers for the openstack > >> stuff, I think that mocking at that level should be sufficient. > > > > > > The various interfaces, as defined, are used in the tests. The test > > suites declare an attribute of the interface type to hold a reference to > > the client impl under test. > > I see that. However, is there any particular reason you can't declare > the interface in the tests rather than muddying the public API? > That's the first time I've heard defining interfaces as "muddying" anything :-) Usually, the interface *is* the public API. But in Go terms, I guess that might be the case? But see my last comment below. > Although I'm not sure that even that is useful - I can't see that using an > interface in the tests rather than the concrete type is gaining anything. > AFAICS your current test-double approach is mocking the http server > rather than these interfaces. I think you could remove the interface > definitions, use the concrete type throughout, and everything would work > just fine. That is true. One other use for defining the interface in the code as that often, that's where the APIs are documented using code comments and provides a nice place for the reader to go to read about the contract that the code implements. It's also what tools use to pull out auto generated documentation etc. If there's no interface, where is the best place to document the API? I guess it can be done on the implementation methods but then it's hard to read because everything is spread out and interspersed with the actual code. And what if there's more than one implementation? Sorry for the all questions. My brain is still converting to Go from other languages.
Sign in to reply to this message.
On 2012/11/28 09:54:52, wallyworld wrote: > On 2012/11/28 09:38:20, rog wrote: > > >> Usually it's not the Go way, because when you test some logic, you > > >> can decide in that place to use an interface or not, and define > > >> the tests and interface for that occasion, rather than defining the > > > > > > interface > > >> > > >> up front. > > > > > > > > >> If you're implementing mock http servers for the openstack > > >> stuff, I think that mocking at that level should be sufficient. > > > > > > > > > The various interfaces, as defined, are used in the tests. The test > > > suites declare an attribute of the interface type to hold a reference to > > > the client impl under test. > > > > I see that. However, is there any particular reason you can't declare > > the interface in the tests rather than muddying the public API? > > > > That's the first time I've heard defining interfaces as "muddying" anything :-) > Usually, the interface *is* the public API. But in Go terms, I guess that might > be the case? But see my last comment below. Yeah, interfaces in Go are used somewhat differently than other languages. > > Although I'm not sure that even that is useful - I can't see that using an > > interface in the tests rather than the concrete type is gaining anything. > > AFAICS your current test-double approach is mocking the http server > > rather than these interfaces. I think you could remove the interface > > definitions, use the concrete type throughout, and everything would work > > just fine. > > That is true. > > One other use for defining the interface in the code as that often, that's where > the APIs are documented using code comments and provides a nice place for the > reader to go to read about the contract that the code implements. That's perhaps the case in other languages. In Go, the convention is a little different. > It's also what > tools use to pull out auto generated documentation etc. If there's no interface, > where is the best place to document the API? The API is conventionally documented on the implementation methods with doc comments. You can use go.pkgdoc.org to see how it looks; for example http://go.pkgdoc.org/launchpad.net/goose/client (You can also run godoc as a web server locally, and use it to generate simple textual documentation). For documentation, your aim should be to make the go.pkgdoc.org pages look good. > I guess it can be done on the > implementation methods but then it's hard to read because everything is spread > out and interspersed with the actual code. In fact, it makes things quite nice IMHO because the doc comments are next to the code they're referring to. And godoc pulls it all out into a reasonable readable web page or text. Avoiding the interface abstraction means that I can click on a method call in the source and it will immediately take me to the method implementation. If the call is to an interface, it can only show me where the interface method is declared - it's another step to find out where that method is implemented. It's not an issue here, but performance is also better when using a type directly - the compiler can inline method calls to a value with a concrete type, for example, but it cannot do that when invoking a method on an interface value. > And what if there's more than one implementation? If you really do have several implementations, then documentation on the interface type is indeed what you want to do (you'd probably hide the concrete type). In this case though, I don't think you're going to have several implementations, and there's no particular benefit that I can see from designing things in advance to that end. If you want to change things later to enable several implementations, that's not hard - you can change the type name and the compiler will tell you everywhere that needs to be changed. > Sorry for the all questions. My brain is still converting to Go from other > languages. No problem! I hope you get to like it as much as we do.
Sign in to reply to this message.
LGTM, overall, with some minor comments - while I agree with some of the other reviews on the interfaces vs. implementations, I still think we'd rather land this and simplify it in a follow up. https://codereview.appspot.com/6782112/diff/8002/glance/glance.go File glance/glance.go (right): https://codereview.appspot.com/6782112/diff/8002/glance/glance.go#newcode28 glance/glance.go:28: func NewClient(client client.Client) Glance { as before, shorter is better - New() https://codereview.appspot.com/6782112/diff/8002/glance/glance_test.go File glance/glance_test.go (right): https://codereview.appspot.com/6782112/diff/8002/glance/glance_test.go#newcode35 glance/glance_test.go:35: s.glance = glance.NewClient(client) glance.New() instead https://codereview.appspot.com/6782112/diff/8002/identity/identity.go File identity/identity.go (right): https://codereview.appspot.com/6782112/diff/8002/identity/identity.go#newcode74 identity/identity.go:74: err = errors.New( probably better use fmt.Errorf() https://codereview.appspot.com/6782112/diff/8002/nova/nova.go File nova/nova.go (right): https://codereview.appspot.com/6782112/diff/8002/nova/nova.go#newcode70 nova/nova.go:70: func NewClient(client client.Client) Nova { just New() https://codereview.appspot.com/6782112/diff/8002/nova/nova_test.go File nova/nova_test.go (right): https://codereview.appspot.com/6782112/diff/8002/nova/nova_test.go#newcode39 nova/nova_test.go:39: s.nova = nova.NewClient(client) nova.New() instead https://codereview.appspot.com/6782112/diff/8002/swift/swift.go File swift/swift.go (right): https://codereview.appspot.com/6782112/diff/8002/swift/swift.go#newcode29 swift/swift.go:29: func NewClient(client client.Client) Swift { swift.New() instead of NewClient() https://codereview.appspot.com/6782112/diff/8002/swift/swift_test.go File swift/swift_test.go (right): https://codereview.appspot.com/6782112/diff/8002/swift/swift_test.go#newcode35 swift/swift_test.go:35: s.swift = swift.NewClient(client) like my comment before - swift.New() instead.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6782112/diff/8002/glance/glance.go File glance/glance.go (right): https://codereview.appspot.com/6782112/diff/8002/glance/glance.go#newcode16 glance/glance.go:16: type Glance interface { On 2012/11/28 01:32:51, dfc wrote: > Tradition dictates that Public symbols are commented, either on the interface > definition, or on the implementation. Yes, true. These first few branches have been more concerned with refactoring (usually with cut and paste) some of the early prototype code into better organised modules, and then a clean up branch would add missing doc etc. I'll see what I can do quickly now but we may still need a followup branch to tidy things up. https://codereview.appspot.com/6782112/diff/8002/glance/glance.go#newcode28 glance/glance.go:28: func NewClient(client client.Client) Glance { On 2012/11/28 15:46:28, dimitern wrote: > as before, shorter is better - New() Done. https://codereview.appspot.com/6782112/diff/8002/glance/glance.go#newcode45 glance/glance.go:45: On 2012/11/28 01:32:51, dfc wrote: > remove space Done. https://codereview.appspot.com/6782112/diff/8002/glance/glance.go#newcode88 glance/glance.go:88: func (c *Client) GetImageDetail(imageId string) (ImageDetail, error) { On 2012/11/28 01:32:51, dfc wrote: > Can you please make the use of named error returns consistent. ie, add them to > this method, or remove them from the previous methods. My preference is not to > use them except when truly needed. Done. https://codereview.appspot.com/6782112/diff/8002/glance/glance_test.go File glance/glance_test.go (right): https://codereview.appspot.com/6782112/diff/8002/glance/glance_test.go#newcode27 glance/glance_test.go:27: c.Fatalf("Error setting up test suite: %s", err.Error()) On 2012/11/28 01:32:51, dfc wrote: > c.Fatalf("Error...: %v", err) > > errors know how to print themselves with %v, however in this case > > c.Assert(err, NotNil) is all you need. Done. https://codereview.appspot.com/6782112/diff/8002/glance/glance_test.go#newcode35 glance/glance_test.go:35: s.glance = glance.NewClient(client) On 2012/11/28 15:46:28, dimitern wrote: > glance.New() instead Done. https://codereview.appspot.com/6782112/diff/8002/glance/glance_test.go#newcode42 glance/glance_test.go:42: c.Assert(err, IsNil) On 2012/11/28 01:32:51, dfc wrote: > c.Assert(len(images), NotEqual, 0) maybe Done. https://codereview.appspot.com/6782112/diff/8002/glance/glance_test.go#newcode43 glance/glance_test.go:43: if len(images) < 1 { On 2012/11/28 01:32:51, dfc wrote: > c.Assert(images, Not(HasLen), 0) Done. https://codereview.appspot.com/6782112/diff/8002/glance/glance_test.go#newcode59 glance/glance_test.go:59: if len(images) < 1 { On 2012/11/28 01:32:51, dfc wrote: > ditto Done. https://codereview.appspot.com/6782112/diff/8002/identity/identity.go File identity/identity.go (right): https://codereview.appspot.com/6782112/diff/8002/identity/identity.go#newcode74 identity/identity.go:74: err = errors.New( On 2012/11/28 15:46:28, dimitern wrote: > probably better use fmt.Errorf() Done. https://codereview.appspot.com/6782112/diff/8002/nova/nova.go File nova/nova.go (right): https://codereview.appspot.com/6782112/diff/8002/nova/nova.go#newcode22 nova/nova.go:22: type Nova interface { On 2012/11/28 01:32:51, dfc wrote: > Please provide a brief comment on each interface method. Done. https://codereview.appspot.com/6782112/diff/8002/nova/nova.go#newcode69 nova/nova.go:69: On 2012/11/28 01:32:51, dfc wrote: > // NewClient returns a new Nova client Isn't that an example of an obvious comment? ie of course NewClient is going to return a new Nova client. https://codereview.appspot.com/6782112/diff/8002/nova/nova.go#newcode70 nova/nova.go:70: func NewClient(client client.Client) Nova { On 2012/11/28 15:46:28, dimitern wrote: > just New() Done. https://codereview.appspot.com/6782112/diff/8002/nova/nova.go#newcode153 nova/nova.go:153: func (c *Client) GetServer(serverId string) (ServerDetail, error) { On 2012/11/28 01:32:51, dfc wrote: > Same comments with respect to named vs unnamed return values. Done. https://codereview.appspot.com/6782112/diff/8002/nova/nova_test.go File nova/nova_test.go (right): https://codereview.appspot.com/6782112/diff/8002/nova/nova_test.go#newcode39 nova/nova_test.go:39: s.nova = nova.NewClient(client) On 2012/11/28 15:46:28, dimitern wrote: > nova.New() instead Done. https://codereview.appspot.com/6782112/diff/8002/swift/swift_test.go File swift/swift_test.go (right): https://codereview.appspot.com/6782112/diff/8002/swift/swift_test.go#newcode25 swift/swift_test.go:25: cred, err := identity.CompleteCredentialsFromEnv() On 2012/11/28 01:32:51, dfc wrote: > c.Assert(err, NotNil) is usually good enough. Done. https://codereview.appspot.com/6782112/diff/8002/swift/swift_test.go#newcode30 swift/swift_test.go:30: err = client.Authenticate() On 2012/11/28 01:32:51, dfc wrote: > c.Assert(err, NotNIl) Done. https://codereview.appspot.com/6782112/diff/8002/swift/swift_test.go#newcode35 swift/swift_test.go:35: s.swift = swift.NewClient(client) On 2012/11/28 15:46:28, dimitern wrote: > like my comment before - swift.New() instead. Done.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
LGTM I'd like to see at least a smoke test for CompleteCredentialsFromEnv (we have the envsuite stuff which should make it reasonably easy to have env isolated test cases.) But I don't really need a re-review vs just landing it when you're happy with it. https://codereview.appspot.com/6782112/diff/3017/identity/identity.go File identity/identity.go (right): https://codereview.appspot.com/6782112/diff/3017/identity/identity.go#newcode77 identity/identity.go:77: } moving this here makes a lot of sense, but since it is exposed, it would seem that we need to add some tests for it in identity/identity_test.go https://codereview.appspot.com/6782112/diff/3017/nova/nova.go File nova/nova.go (left): https://codereview.appspot.com/6782112/diff/3017/nova/nova.go#oldcode90 nova/nova.go:90: I don't really understand why Roger doesn't like named return variables. I personally think they are really nice. I like named parameters and return variables because it tends to be helpful for readers of the function. Roger, can you comment on why you recommended removing them all? I'm assuming that the changes in this file are all just the mechanical "get rid of named return values and make them explicit". Maybe he is objecting to mixing the two styles? (naming the return values but explicitly returning 'return resp.Flavors, err' rather than: flavors = resp.Flavors return
Sign in to reply to this message.
*** Submitted: Initial glance client This branch implements an initial glance client. Only a few basic APIs are implemented since I'm not sure exactly what we'll need to provide. New APIs can easily be added as required. I've also renamed a few interfaces. R=jameinel, rog, gz, dfc, dimitern CC= https://codereview.appspot.com/6782112
Sign in to reply to this message.
https://codereview.appspot.com/6782112/diff/3017/nova/nova.go File nova/nova.go (left): https://codereview.appspot.com/6782112/diff/3017/nova/nova.go#oldcode90 nova/nova.go:90: On 2012/11/29 06:10:00, jameinel wrote: > I don't really understand why Roger doesn't like named return variables. I > personally think they are really nice. I like named parameters and return > variables because it tends to be helpful for readers of the function. > > Roger, can you comment on why you recommended removing them all? I'm assuming > that the changes in this file are all just the mechanical "get rid of named > return values and make them explicit". > > Maybe he is objecting to mixing the two styles? (naming the return values but > explicitly returning 'return resp.Flavors, err' rather than: > flavors = resp.Flavors > return this post probably says it better than i would: https://plus.google.com/106356964679457436995/posts/LmnDfgehorU in this particular case, it's not obvious that the function always returns a nil flavors value if it returns an error. in fact, i believe that in certain (admittedly unusual) cases it may not, as json decoding can return an error while still filling in some of its parameters. for example: http://play.golang.org/p/BD8mLD0hta https://codereview.appspot.com/6782112/diff/3017/nova/nova.go File nova/nova.go (right): https://codereview.appspot.com/6782112/diff/3017/nova/nova.go#newcode24 nova/nova.go:24: type Client struct { thanks. https://codereview.appspot.com/6782112/diff/3017/nova/nova.go#newcode32 nova/nova.go:32: type Link struct { in another CL, i'd still like to see some documentation on this type and its fields. is Href a URL and Rel the relative part of it. what kind of things might i expect to find in Type? as someone that might use this API at some stage, it would be nice if i could use it without needing to read the entire openstack API documentation. for instance, the goamz docs have a URL in the docs for each entry point, and that works really well. https://codereview.appspot.com/6782112/diff/3017/nova/nova.go#newcode52 nova/nova.go:52: err := c.client.SendRequest(client.GET, "compute", apiFlavors, &requestData, "failed to get list of flavors") i still think this should be: if err != nil { return nil, err } return resp.Flavors, nil but it's fine to do it later.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
