Code review comment for lp:~allenap/gwacl/set-network-configuration

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

 review: approve
 merge: approve

Nice change, thank you. I'll land it right away, if there's anything
coming out of my questions below it can get fixed later.

On 11/07/13 03:11, Gavin Panella wrote:
> - Azure doesn't actually like the Label tag in VirtualNetworkSite
> elements, even though the example operation docs show it.

Can you file a bug on ***sing please, they asked if we could do this
for any doc problems.

> - Built on the work Julian did on the example to create an
> affinity group and virtual network, then tear it down again. This
> is a work in progress; the deployed VMs are not using the virtual
> network or affinity group, and the network config gets clobbered.
> However, these are tasks for a subsequent branch.

That's cool, at least it shows the actual API calls work.

> === modified file 'example/management/run.go'

Gosh those changes looked familiar :)

> === modified file 'management_base_test.go' ---
> management_base_test.go 2013-07-10 10:08:25 +0000 +++
> management_base_test.go 2013-07-10 17:10:42 +0000 @@ -929,7 +929,6
> @@ { Name: "virtual-network-name", AffinityGroup:
> "affinity-group-name", - Label:
> "label-for-the-site", AddressSpacePrefixes: []string{
> "CIDR-identifier", }, @@ -944,7 +943,7 @@ Name:
> "primary-DNS-name", }, }, - Gateway: Gateway{ +
> Gateway: &Gateway{

Urgh, the fact that we have to work around the Go omitempty bug with
pointers like is particularly f'in annoying as it leaves our
structures inconsistently presented.

> === modified file 'x509dispatcher.go' --- x509dispatcher.go
> 2013-07-01 13:04:17 +0000 +++ x509dispatcher.go 2013-07-10 17:10:42
> +0000 @@ -61,12 +61,13 @@
>
> // newX509RequestPUT initializes an X509Request for a PUT. You may
> still // need to set further values. -func newX509RequestPUT(url
> string, payload []byte) *X509Request { +func newX509RequestPUT(url
> string, payload []byte, contentType string) *X509Request { return
> &X509Request{ - Method: "PUT", - URL:
> url, - APIVersion: baseAPIVersion, - Payload:
> payload, + Method: "PUT", + URL: url, +
> APIVersion: baseAPIVersion, + Payload: payload, +
> ContentType: contentType,

Sigh, gofmt, SIGH.

> } }
>
> @@ -201,7 +202,7 @@ if len(request.Payload) != 0 { headers =
> append(headers, "Content-Type: "+request.ContentType) // Arrange
> for the request body to be written. -
> ch.Setopt(curl.OPT_POSTFIELDSIZE, len(request.Payload)) +
> headers = append(headers, "Transfer-Encoding: chunked")
> ch.Setopt(curl.OPT_READFUNCTION, func(buf []byte, _ interface{})
> int { // Buffered request write. Copy as much of the payload as
> will // fit into the buffer curl gave us for it, and return the
> number @@ -242,7 +243,7 @@ case "POST": ch.Setopt(curl.OPT_POST,
> true) case "PUT": - ch.Setopt(curl.OPT_CUSTOMREQUEST,
> "PUT") + ch.Setopt(curl.OPT_UPLOAD, true)

Out of interest, where is PUT set now? Does OPT_UPLOAD imply PUT?
(Sorry, I know bugger all about curl)

> case "DELETE": ch.Setopt(curl.OPT_CUSTOMREQUEST, "DELETE")
> default:
>
> === modified file 'x509dispatcher_test.go' ---
> x509dispatcher_test.go 2013-07-02 07:15:53 +0000 +++
> x509dispatcher_test.go 2013-07-10 17:10:42 +0000 @@ -1,6 +1,7 @@
> package gwacl
>
> import ( + "io/ioutil" . "launchpad.net/gocheck" "net/http"
> "net/http/httptest" @@ -10,13 +11,24 @@
>
> var _ = Suite(&x509DispatcherSuite{})
>
> +type Request struct { + *http.Request + BodyContent []byte
> +} + // makeRecordingHTTPServer creates an http server (don't
> forget to Close() it when done) // that serves at the given base
> URL, copies incoming requests into the given // channel, and
> finally returns the given status code. If body is not nil, it //
> will be returned as the request body. -func
> makeRecordingHTTPServer(requests chan *http.Request, status int,
> body []byte, headers http.Header) *httptest.Server { +func
> makeRecordingHTTPServer(requests chan *Request, status int, body
> []byte, headers http.Header) *httptest.Server { returnRequest :=
> func(w http.ResponseWriter, r *http.Request) { - requests <-
> r + // Capture all the request body content for later
> inspection. + requestBody, err := ioutil.ReadAll(r.Body)

Doesn't this need the NopCloser trick? Or is r.Body still readable later?

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlHeKKQACgkQWhGlTF8G/HficgCfdYim3CPPaIgKL6ijJu3OfBRK
+54AniW5Z5bEHxlVnQreqm3qJM2o2bt9
=n9CM
-----END PGP SIGNATURE-----