Merge lp:~allenap/gwacl/remove-role-endpoints-2 into lp:gwacl

Proposed by Gavin Panella on 2013-07-25
Status: Merged
Approved by: Gavin Panella on 2013-07-26
Approved revision: 204
Merged at revision: 203
Proposed branch: lp:~allenap/gwacl/remove-role-endpoints-2
Merge into: lp:gwacl
Diff against target: 69 lines (+4/-16)
2 files modified
example/management/run.go (+3/-9)
x509dispatcher.go (+1/-7)
To merge this branch: bzr merge lp:~allenap/gwacl/remove-role-endpoints-2
Reviewer Review Type Date Requested Status
Julian Edwards (community) 2013-07-25 Approve on 2013-07-26
Review via email: mp+177065@code.launchpad.net

Commit message

Niceties missed from the main remove-role-endpoints branch.

To post a comment you must log in.
Julian Edwards (julian-edwards) wrote :

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

 review: approve

Approved but I'm interested to know why you changed a couple of things!

> defer func() { Info("Removing role input endpoint...") - //
> FIXME: Something above - AddRoleEndpoints seems a prime suspect - -
> // causes Azure to do something asynchronously, which we must wait
> for - // before attempting this call, or it will blow up
> with a 409.

Was this fixed previously then? Or was it not a problem?

> -func newX509Response() *x509Response { - return &x509Response{
> - Body: make([]byte, 0), - } -} -
[snip]
> - response := newX509Response() + response :=
> &x509Response{} response.StatusCode = httpResponse.StatusCode
> response.Body, err = ioutil.ReadAll(httpResponse.Body) if err !=
> nil {
>

This is removing a make() on the Body - is that right/ok? Go still
leaves me wondering whether it has changed the semantics when you
assign to Body in the second-last line here ... :/

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

iEYEARECAAYFAlHx4mAACgkQWhGlTF8G/HdkIwCgpzEEtzP+ZKpCNC68JEo9+a9t
1csAn1YEXFNDv2Dx/Iz3qoUbMJP2Jnc2
=IDOH
-----END PGP SIGNATURE-----

review: Approve
Julian Edwards (julian-edwards) wrote :

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

Sigh @ whatever screwed up formatting on the code quote parts.
/me stares at Thunderbird
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlHx42IACgkQWhGlTF8G/Hd6cQCgpixY0qeaTOaxUy1wiX/lcm79
vWEAoKWUMqX4a/BMOQu7BuisM8yykVXd
=xBSW
-----END PGP SIGNATURE-----

Gavin Panella (allenap) wrote :

>> defer func() { Info("Removing role input endpoint...") - //
>> FIXME: Something above - AddRoleEndpoints seems a prime suspect - -
>> // causes Azure to do something asynchronously, which we must wait
>> for - // before attempting this call, or it will blow up
>> with a 409.
>
> Was this fixed previously then? Or was it not a problem?

I added this in my previous branch, but also fixed it there and forgot
to remove this message.

>
>> -func newX509Response() *x509Response { - return &x509Response{
>> - Body: make([]byte, 0), - } -} -
> [snip]
>> - response := newX509Response() + response :=
>> &x509Response{} response.StatusCode = httpResponse.StatusCode
>> response.Body, err = ioutil.ReadAll(httpResponse.Body) if err !=
>> nil {
>>
>
> This is removing a make() on the Body - is that right/ok? Go still
> leaves me wondering whether it has changed the semantics when you
> assign to Body in the second-last line here ... :/

Yeah, it's not needed. The zero value for x509Response.Body is an
empty slice.

Thanks for the review!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'example/management/run.go'
2--- example/management/run.go 2013-07-25 21:07:12 +0000
3+++ example/management/run.go 2013-07-25 22:26:30 +0000
4@@ -76,9 +76,6 @@
5 api, err := gwacl.NewManagementAPI(subscriptionID, certFile)
6 checkError(err)
7
8- // Exercise the API.
9-
10- // Exercise hosted services API.
11 ExerciseHostedServicesAPI(api)
12
13 Info("All done.")
14@@ -172,7 +169,7 @@
15 deployments, err := api.ListAllDeployments(&gwacl.ListAllDeploymentsRequest{ServiceName: hostedService.ServiceName})
16 checkError(err)
17 if len(deployments) != len(detailedHostedService.Deployments) {
18- Infof(
19+ Errorf(
20 "Mismatch in reported deployments: %d != %d",
21 len(deployments), len(detailedHostedService.Deployments))
22 }
23@@ -260,9 +257,6 @@
24
25 defer func() {
26 Info("Removing role input endpoint...")
27- // FIXME: Something above - AddRoleEndpoints seems a prime suspect -
28- // causes Azure to do something asynchronously, which we must wait for
29- // before attempting this call, or it will blow up with a 409.
30 err := api.RemoveRoleEndpoints(&gwacl.RemoveRoleEndpointsRequest{
31 ServiceName: hostServiceName,
32 DeploymentName: deployment.Name,
33@@ -275,8 +269,8 @@
34
35 if pause {
36 var wait string
37- Info("Pausing so you can play with the newly-created VM")
38- Info("To clear up, type something and press enter to continue")
39+ fmt.Println("Pausing so you can play with the newly-created VM")
40+ fmt.Println("To clear up, type something and press enter to continue")
41 fmt.Scan(&wait)
42 }
43 }
44
45=== modified file 'x509dispatcher.go'
46--- x509dispatcher.go 2013-07-25 17:31:17 +0000
47+++ x509dispatcher.go 2013-07-25 22:26:30 +0000
48@@ -65,12 +65,6 @@
49 Header http.Header
50 }
51
52-func newX509Response() *x509Response {
53- return &x509Response{
54- Body: make([]byte, 0),
55- }
56-}
57-
58 func performX509Request(session *x509Session, request *X509Request) (*x509Response, error) {
59 Debugf("Request: %s %s", request.Method, request.URL)
60 if len(request.Payload) > 0 {
61@@ -89,7 +83,7 @@
62 return nil, err
63 }
64
65- response := newX509Response()
66+ response := &x509Response{}
67 response.StatusCode = httpResponse.StatusCode
68 response.Body, err = ioutil.ReadAll(httpResponse.Body)
69 if err != nil {

Subscribers

People subscribed via source and target branches

to all changes: