Merge lp:~allenap/gwacl/list-instances-2 into lp:gwacl

[Gavin Panella]

In ListInstances, only return instances for a single service.

Previously it returned instances for all services.

[Raphaël Badin (rvb)]

Looks good.

I see you've changed ListInstances() to take a struct instead of the hosted service name as a string like all the other methods in the management API. Now I'm on the fence with this: I think we should change all the methods to take structs but maybe that's a change we should do all at once; on the other hand, starting to do it right now with new methods will save us time updating call sites in the provider later; and given how painful it is to juggle with dependencies versions in Go, maybe your choice is the right one after all…

[0]

> 216 lines (+26/-119) 3 files modified

Nice :)

[1]

30 + ServiceName string
31 +}
32 +
33 +// ListInstances returns a slice of all instances for all deployments for the
34 +// given service.

I'd keep saying "hosted service name" instead of just "service name" for consistency (and more importantly because that's how the doc refers to a service).

review: Approve

[lp:~allenap/gwacl/list-instances-2]

132. By Gavin Panella on 2013-06-24

Update docstring.

[Gavin Panella (allenap)]

> I see you've changed ListInstances() to take a struct instead of the hosted
> service name as a string like all the other methods in the management API.
> Now I'm on the fence with this: I think we should change all the methods to
> take structs but maybe that's a change we should do all at once; on the other
> hand, starting to do it right now with new methods will save us time updating
> call sites in the provider later; and given how painful it is to juggle with
> dependencies versions in Go, maybe your choice is the right one after all…

I think we're safe to do it bit by bit, and I would prefer to do it
that way. There's no need to do it all at once, so we shouldn't force
ourselves to do it all at once. Go will not let us pass the wrong
thing so we'll have early warning when we've got it wrong, and how.

> [1]
>
> 30      +    ServiceName string
> 31      +}
> 32      +
> 33      +// ListInstances returns a slice of all instances for all deployments
> for the
> 34      +// given service.
>
> I'd keep saying "hosted service name" instead of just "service name" for
> consistency (and more importantly because that's how the doc refers to a
> service).

Changed.

Thanks for the review!

=== modified file 'example/management/run.go'
--- example/management/run.go	2013-06-19 21:01:54 +0000
+++ example/management/run.go	2013-06-24 16:05:33 +0000
@@ -148,6 +148,12 @@
     checkError(err)
     fmt.Println("Done starting VM\n")
 
+    fmt.Println("Listing VM...")
+    instances, err := api.ListInstances(&gwacl.ListInstancesRequest{hostServiceName})
+    checkError(err)
+    fmt.Printf("Got %d instance(s)\n", len(instances))
+    fmt.Println("Done listing VM\n")
+
     fmt.Println("Stopping VM...")
     err = api.PerformRoleOperation(hostServiceName, deployment.Name, role.RoleName, gwacl.ShutdownRoleOperation)
     checkError(err)

=== modified file 'management.go'
--- management.go	2013-06-21 15:14:41 +0000
+++ management.go	2013-06-24 16:05:33 +0000
@@ -3,22 +3,20 @@
 
 package gwacl
 
-// ListInstances returns a slice of all instances for all deployments for all
-// services reachable with the user's credentials.
-func (api *ManagementAPI) ListInstances() ([]RoleInstance, error) {
-    services, err := api.ListHostedServices()
+type ListInstancesRequest struct {
+    ServiceName string
+}
+
+// ListInstances returns a slice of all instances for all deployments for the
+// given hosted service name.
+func (api *ManagementAPI) ListInstances(request *ListInstancesRequest) ([]RoleInstance, error) {
+    instances := []RoleInstance{}
+    properties, err := api.GetHostedServiceProperties(request.ServiceName, true)
     if err != nil {
         return nil, err
     }
-    instances := []RoleInstance{}
-    for _, service := range services {
-        properties, err := api.GetHostedServiceProperties(service.ServiceName, true)
-        if err != nil {
-            return nil, err
-        }
-        for _, deployment := range properties.Deployments {
-            instances = append(instances, deployment.RoleInstanceList...)
-        }
+    for _, deployment := range properties.Deployments {
+        instances = append(instances, deployment.RoleInstanceList...)
     }
     return instances, nil
 }

=== modified file 'management_test.go'
--- management_test.go	2013-06-22 14:08:31 +0000
+++ management_test.go	2013-06-24 16:05:33 +0000
@@ -14,22 +14,7 @@
 
 // TestListInstances goes through the happy path for ListInstances.
 func (suite *managementAPISuite) TestListInstances(c *C) {
-    // First response is the list of services.
-    services := HostedServiceDescriptorList{
-        HostedServices: []HostedServiceDescriptor{
-            {ServiceName: "S1"},
-            {ServiceName: "S2"},
-        },
-    }
-    servicesXML, err := services.Serialize()
-    c.Assert(err, IsNil)
-    servicesResponse := DispatcherResponse{
-        response: &x509Response{
-            Body:       []byte(servicesXML),
-            StatusCode: http.StatusOK,
-        },
-    }
-    // Second response is the list of properties for "S1".
+    // The list of properties for "S1".
     propertiesS1 := HostedService{
         ServiceName: "S1",
         Deployments: []Deployment{
@@ -55,125 +40,43 @@
             StatusCode: http.StatusOK,
         },
     }
-    // Third response is the list of properties for "S2".
-    propertiesS2 := HostedService{
-        ServiceName: "S2",
-        Deployments: []Deployment{
-            {
-                RoleInstanceList: []RoleInstance{
-                    {RoleName: "five"},
-                    {RoleName: "six"},
-                },
-            },
-            {
-                RoleInstanceList: []RoleInstance{
-                    {RoleName: "seven"},
-                    {RoleName: "eight"},
-                },
-            },
-        },
-    }
-    propertiesS2XML, err := propertiesS2.Serialize()
-    c.Assert(err, IsNil)
-    propertiesS2Response := DispatcherResponse{
-        response: &x509Response{
-            Body:       []byte(propertiesS2XML),
-            StatusCode: http.StatusOK,
-        },
-    }
-    // Rig the prepared responses in.
+    // Rig the prepared response in.
     record := []*x509Request{}
     rigRecordingPreparedResponseDispatcher(
-        &record, []DispatcherResponse{
-            servicesResponse,
-            propertiesS1Response,
-            propertiesS2Response,
-        },
+        &record, []DispatcherResponse{propertiesS1Response},
     )
     // Finally, exercise ListInstances.
     api := makeAPI(c)
-    instances, err := api.ListInstances()
+    request := &ListInstancesRequest{ServiceName: propertiesS1.ServiceName}
+    instances, err := api.ListInstances(request)
     c.Assert(err, IsNil)
     c.Check(instances, DeepEquals, []RoleInstance{
         propertiesS1.Deployments[0].RoleInstanceList[0],
         propertiesS1.Deployments[0].RoleInstanceList[1],
         propertiesS1.Deployments[1].RoleInstanceList[0],
         propertiesS1.Deployments[1].RoleInstanceList[1],
-        propertiesS2.Deployments[0].RoleInstanceList[0],
-        propertiesS2.Deployments[0].RoleInstanceList[1],
-        propertiesS2.Deployments[1].RoleInstanceList[0],
-        propertiesS2.Deployments[1].RoleInstanceList[1],
     })
-    // The first request was for the services.
+    // The only request is for S1's properties
     c.Assert(len(record), Not(Equals), 0)
     c.Check(record[0], DeepEquals, &x509Request{
         APIVersion: baseAPIVersion,
-        URL:        AZURE_URL + "subscriptionId/services/hostedservices",
-        Method:     "GET",
-    })
-    // The second for S1's properties
-    c.Assert(len(record), Not(Equals), 1)
-    c.Check(record[1], DeepEquals, &x509Request{
-        APIVersion: baseAPIVersion,
         URL:        AZURE_URL + "subscriptionId/services/hostedservices/S1?embed-detail=true",
         Method:     "GET",
     })
-    // The third for S2's properties
-    c.Assert(len(record), Not(Equals), 2)
-    c.Check(record[2], DeepEquals, &x509Request{
-        APIVersion: baseAPIVersion,
-        URL:        AZURE_URL + "subscriptionId/services/hostedservices/S2?embed-detail=true",
-        Method:     "GET",
-    })
-}
-
-func (suite *managementAPISuite) TestListInstancesFailsGettingServices(c *C) {
-    rigPreparedResponseDispatcher(
-        []DispatcherResponse{
-            {
-                response: &x509Response{
-                    StatusCode: http.StatusUnauthorized,
-                },
-            },
-        },
-    )
-    api := makeAPI(c)
-    instances, err := api.ListInstances()
-    c.Check(instances, IsNil)
-    c.Assert(err, NotNil)
-    c.Assert(err.Error(), Equals, "GET request failed (401: Unauthorized)")
 }
 
 func (suite *managementAPISuite) TestListInstancesFailsGettingDetails(c *C) {
-    // First response is the list of services.
-    services := HostedServiceDescriptorList{
-        HostedServices: []HostedServiceDescriptor{
-            {ServiceName: "S1"},
-        },
-    }
-    servicesXML, err := services.Serialize()
-    c.Assert(err, IsNil)
-    servicesResponse := DispatcherResponse{
-        response: &x509Response{
-            Body:       []byte(servicesXML),
-            StatusCode: http.StatusOK,
-        },
-    }
-    // Second response to the properties request is Not Found, to simulate a
-    // race perhaps.
     propertiesS1Response := DispatcherResponse{
         response: &x509Response{
             StatusCode: http.StatusNotFound,
         },
     }
     rigPreparedResponseDispatcher(
-        []DispatcherResponse{
-            servicesResponse,
-            propertiesS1Response,
-        },
+        []DispatcherResponse{propertiesS1Response},
     )
     api := makeAPI(c)
-    instances, err := api.ListInstances()
+    request := &ListInstancesRequest{ServiceName: "SomeService"}
+    instances, err := api.ListInstances(request)
     c.Check(instances, IsNil)
     c.Assert(err, NotNil)
     c.Assert(err.Error(), Equals, "GET request failed (404: Not Found)")