/goose

Added support for Cinder. #3

Merged
merged 3 commits into from Mar 24, 2015

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@kat-co @axw @dimitern @jujubot @wallyworld
@kat-co
Contributor

kat-co commented Mar 8, 2015

No description provided.

cinder/autogenerated_client.go
@@ -0,0 +1,1821 @@
+package cinder
@axw

axw Mar 9, 2015

Member

All source files need a copyright header.

@kat-co

kat-co Mar 9, 2015

Contributor

Corrected.

Just FYI, it looks like no other files have the copyright. We might want a separate PR to correct that.

@axw

axw Mar 16, 2015

Member

Oops. Yes.

cinder/client.go
+
+type TokenFn func() string
+
+func SetEndpointFn(endpoint *url.URL, wrappedHandler RequestHandlerFn) RequestHandlerFn {
@axw

axw Mar 9, 2015

Member

Seeing as the Host field is the only part of endpoint that is used, just pass in host?

@axw

axw Mar 9, 2015

Member

This should be documented.

@kat-co

kat-co Mar 9, 2015

Contributor

Documentation corrected.

Regarding the endpoint parameter, I had this same debate with myself. The reason I went with URL is because I wanted a well-formed host, and URL guarantees this. I could take in a string and validate in this function, but that breaks the desired signature by returning an error (this makes it less "nice" to chain wrapper functions).

It's definitely debatable... given that new information, do you have any opinions/suggestions?

cinder/client.go
+ "time"
+)
+
+type TokenFn func() string
@axw

axw Mar 9, 2015

Member

doc

@kat-co

kat-co Mar 9, 2015

Contributor

Corrected.

cinder/client.go
+ }
+}
+
+func SetAuthHeaderFn(token TokenFn, wrappedHandler RequestHandlerFn) RequestHandlerFn {
@axw

axw Mar 9, 2015

Member

doc

@kat-co

kat-co Mar 9, 2015

Contributor

Corrected.

cinder/client.go
+ handleRequest RequestHandlerFn
+}
+
+// Shows information for a specified snapshot.
@axw

axw Mar 9, 2015

Member

These aren't auto-generated, right? Would prefer to stick with conventional "GetSnapshot ..."

@kat-co

kat-co Mar 9, 2015

Contributor

No they are not. Apologies, I think I just copied the documentation from the WADL and forgot to convert to Go format.

cinder/client.go
+
+type StatusResultFn func() (string, error)
+
+// VolumeStatusNotifier will check a volume's status to determine
@axw

axw Mar 9, 2015

Member

doc comment is inconsistent with method name

@kat-co

kat-co Mar 9, 2015

Contributor

Corrected.

cinder/client.go
+// "waitDur" before attempting again. If the status does not match
+// after "numAttempts", an error is returned.
+func (c *Client) StatusNotifier(
+ getStatus StatusResultFn,
@axw

axw Mar 9, 2015

Member

I suspect the two first params would be better as a single predicate that returns (bool, error). That'll allow for alternative strategies, like waiting for the status to be in a set, or not equal to something, etc.

@kat-co

kat-co Mar 9, 2015

Contributor

Excellent suggestion!

cinder/autogenerated_client.go
+
+//
+// Updates a volume type.
+func updateVolumeType(request RequestHandlerFn, args UpdateVolumeTypeParams) (*UpdateVolumeTypeResults, error) {
@axw

axw Mar 9, 2015

Member

I wonder if these shouldn't be exported. We might periodically regenerate these, but not add exported methods on Client for every function. Then, others can make use of these auto-generated functions without having to go through the process of adding an exported method, writing tests, etc.

@wallyworld

wallyworld Mar 9, 2015

Owner

In my view, I'd like to refrain from exporting these at the moment. The user facing, exported functions take params divorced from the params structs used on the wire etc. I think it's good to be able to explicitly control the user facing interface.

@kat-co

kat-co Mar 9, 2015

Contributor

@axw: Originally I did have these exported. I agree that it would probably be helpful to give the user flexibility in how they interact with the API.

@wallyworld: These are the same types that are utilized in the public functions, and they will always represent the information the user must specify in order to make a request. I agree that we should offer a curated experience, but I think I disagree that we should try and hide the raw calls from developers. If we make a breaking change, we can always use a version bump.

I am open to this idea. Can you give me an example of where this might be problematic to help clarify?

@axw

axw Mar 9, 2015

Member

@katco-: If you do go ahead and export them, I think it'd be nice to have the generated and hand-written code in separate packages. Along the lines of the "gen" subpackage in https://github.com/awslabs/aws-sdk-go. Doing that would make it very clear that the functions are auto-generated, as opposed to part of a well-tested, curated set of functions.

@kat-co

kat-co Mar 17, 2015

Contributor

@wallyworld and I discussed this at one point. I think it's a good idea, and the only reason I can see to keep it in the same package is that we make extensive use of the autogenerated types. I'm not sure if it makes sense to have to reference a 2nd package for that. Any opinion on that?

@axw

axw Mar 17, 2015

Member

Yes, that would be awkward. Let's leave this for now.

cinder/autogenerated_client.go
+ "strings"
+)
+
+type RequestHandlerFn func(*http.Request) (*http.Response, error)
@axw

axw Mar 9, 2015

Member

It'd be good to document this, and describe that the auto-generated functions will have a hard-coded URL that needs to be overridden by the RequestHandlerFn. Maybe move it out of the auto-generated file as well.

@wallyworld

wallyworld Mar 9, 2015

Owner

I'd like to see a bunch of doc describing the auto generation process, a link to the generation utility, where the WADL is sourced from etc.

@kat-co

kat-co Mar 16, 2015

Contributor

I've added a bunch of documentation at the top.

cinder/client.go
+) <-chan error {
+ notifierChan := make(chan error)
+ go func() {
+ for attemptNum := 0; attemptNum < numAttempts; attemptNum++ {
@axw

axw Mar 9, 2015

Member

defer close(notifierChan) prior to the for loop
No need for the notifierChan <- nil below then.

@kat-co

kat-co Mar 9, 2015

Contributor

Great suggestion!

@axw
Member

axw commented Mar 9, 2015

Generally looking good, just a few comments. I'd like to see the code generator checked into go-goose (or a separate repo), and a //go:generate directive added to one of the Go files to create autogenerated_client.go.
client/client.go
@@ -146,6 +152,11 @@ func (c *client) MakeServiceURL(serviceType string, parts []string) (string, err
return makeURL(c.baseURL, parts), nil
}
+func (c *authenticatingClient) HostForRegion(region string) string {
+ return ""
+ //return c.regionServiceURLs[region]
@wallyworld

wallyworld Mar 9, 2015

Owner

?

@kat-co

kat-co Mar 9, 2015

Contributor

Whoops, development artifact! I've removed this entire function.

@dimitern

dimitern commented Mar 9, 2015

@katco- do you plan on adding a cinder test double package later to support local/live tests needed for juju, like we have for nova and others?
@kat-co
Implemented review suggestion. 
- Added a new "Basic" client ctor to ease basic use-cases and serve as an example for the command pattern.
- Cleaned up documentation and erroneous development artifacts.
e1a99cf
@kat-co
Contributor

kat-co commented Mar 9, 2015

@axw Regarding the auto-generation, and the inclusion of a go:generate directive. I'm going to look into this a bit, but one thing that immediately strikes me as problematic is referencing the external WADL files. I'm not immediately sure how to responsibly do that. Vendoring seems problematic at first glance.
@axw
Member

axw commented Mar 9, 2015

@axw Regarding the auto-generation, and the inclusion of a go:generate directive. I'm going to look into this a bit, but one thing that immediately strikes me as problematic is referencing the external WADL files. I'm not immediately sure how to responsibly do that. Vendoring seems problematic at first glance.

Is there a URL for the WADL file(s)? The generate command can be arbitrarily complex, so it can grab external files if necessary.

I'm not sure about vendoring. go-aws does do that, from what it looks like. It may also make sense to vendor the version of the WADL files that were used to generate the source files in the repo.
cinder/autogenerated_client.go
@@ -0,0 +1,1704 @@
+// Copyright 2015 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
@axw

axw Mar 16, 2015

Member

Eep. Libraries should be using LGPLv3, and the root LICENSE file confirms that.

@kat-co

kat-co Mar 17, 2015

Contributor

Ack! Good catch. Thank you!

@axw
Member

axw commented Mar 16, 2015

With license header fix, LGTM.
@dimitern

dimitern commented Mar 18, 2015

@kat-co let's try merging this with the bot! :)
nova/nova.go
+ err := c.client.SendRequest(client.POST, "compute", url, &requestData)
+ if errors.IsNotFound(err) {
+ return nil, errors.NewNotImplementedf(
+ err, nil, "the server does not support availability zones",
@wallyworld

wallyworld Mar 24, 2015

Owner

I think this is a cut and paste snafu.

@kat-co

kat-co Mar 24, 2015

Contributor

Correct you are!

nova/nova.go
+ VolumeId string `json:"volumeId"`
+}
+
+func (c *Client) AttachVolume(serverId, volumeId string) (*VolumeAttachment, error) {
@wallyworld

wallyworld Mar 24, 2015

Owner

Can we add a comment

@kat-co

kat-co Mar 24, 2015

Contributor

Added comments on both VolumeAttachment and AttachVolume.

nova/nova.go
@@ -706,3 +707,32 @@ func (c *Client) ListAvailabilityZones() ([]AvailabilityZone, error) {
}
return resp.AvailabilityZoneInfo, nil
}
+
+func (c *Client) ListExtensions() {
@wallyworld

wallyworld Mar 24, 2015

Owner

Is this intentional?

@kat-co

kat-co Mar 24, 2015

Contributor

Nope; removed.

@kat-co
Minor improvements. 
- Improved documentation on autogenerated client.
- Autogenerated client now has better response structures. This also simplified tests.
- Added support for attaching volumes through Nova.
- Added support to the Nova test double for attaching volumes.
0fd8186
@kat-co
Contributor

kat-co commented Mar 24, 2015

$$merge$$
@jujubot
Member

jujubot commented Mar 24, 2015

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-goose
@jujubot
Member

jujubot commented Mar 24, 2015

Build failed: Merging failed
build url: http://juju-ci.vapour.ws:8080/job/github-merge-goose/2

wallyworld added a commit that referenced this pull request Mar 24, 2015

@wallyworld
Merge pull request #3 from kat-co/cinder-support 
Added support for Cinder.
ff89212

@wallyworld wallyworld merged commit ff89212 into go-goose:v1 Mar 24, 2015

@hmlanigan hmlanigan referenced this pull request May 9, 2017

Closed

apiversion_test has failures #49

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment