Merge lp:~rvb/gwacl/list-endpoints into lp:gwacl

[Raphaël Badin]

Add ListRoleEndpoints method.

[Gavin Panella (allenap)]

[1]

+            endpointsP := vmRole.ConfigurationSets[i].InputEndpoints

I assume the P suffix denotes a pointer. I'm not sure I like that, but
don't change it; I don't have a reason to object other than aesthetics
really.

[2]

+func (suite *suiteListRoleEndpoints) TestWhenGetRoleFails(c *C) {
...
+    c.Check(err, ErrorMatches, "GET request failed [(]404: Not Found[)]")

I wonder if we should revisit these behaviours everywhere in GWACL and
return nil instead of an error. That's easier to use in a conditional,
and a common idiom for absense of something. Errors are a pain to make
decisions about, except nil or not. What do you think? (Utterly out of
scope for this branch; I'm just seeking your opinion.)

review: Approve

[Julian Edwards (julian-edwards)]

On Monday 05 Aug 2013 21:52:27 Gavin Panella wrote:
> Review: Approve
>
> [1]
>
> + endpointsP := vmRole.ConfigurationSets[i].InputEndpoints
>
> I assume the P suffix denotes a pointer. I'm not sure I like that, but
> don't change it; I don't have a reason to object other than aesthetics
> really.

In the olden (some say golden) days of C and C++, we used to have pMyVariable. If
thingP is idiomatic, I wonder what motivated Pike and Co. to do it differently? I am
guessing the same reason they made a bunch of other things unnecessarily
different.

>
>
> [2]
>
> +func (suite *suiteListRoleEndpoints) TestWhenGetRoleFails(c *C) {
> ...
> + c.Check(err, ErrorMatches, "GET request failed [(]404: Not Found[)]")
>
> I wonder if we should revisit these behaviours everywhere in GWACL and
> return nil instead of an error. That's easier to use in a conditional,
> and a common idiom for absense of something. Errors are a pain to make
> decisions about, except nil or not. What do you think? (Utterly out of
> scope for this branch; I'm just seeking your opinion.)

I think there's an argument both ways here. Returning a nil is useful in the scenario
you describe, for sure. Conversely, I am loathe to remove information from the
caller. Gwacl needs to be a bit lower-layer than that, or its use starts to get biased
to a certain kind of application.

I do like the idea of the "wrapper" functions in the non "_base" files. Perhaps we
just need more of those instead?

[Jeroen T. Vermeulen (jtv)]

> > I assume the P suffix denotes a pointer. I'm not sure I like that, but
> > don't change it; I don't have a reason to object other than aesthetics
> > really.
>
> In the olden (some say golden) days of C and C++, we used to have pMyVariable.
> If
> thingP is idiomatic, I wonder what motivated Pike and Co. to do it
> differently? I am
> guessing the same reason they made a bunch of other things unnecessarily
> different.

Traditionally, a P as a suffix denotes a Boolean. It stands for Predicate.

[Raphaël Badin (rvb)]

> [2]
>
> +func (suite *suiteListRoleEndpoints) TestWhenGetRoleFails(c *C) {
> ...
> +    c.Check(err, ErrorMatches, "GET request failed [(]404: Not Found[)]")
>
> I wonder if we should revisit these behaviours everywhere in GWACL and
> return nil instead of an error. That's easier to use in a conditional,
> and a common idiom for absense of something. Errors are a pain to make
> decisions about, except nil or not. What do you think? (Utterly out of
> scope for this branch; I'm just seeking your opinion.)

You certainly have a point here… but, as Julian pointed out, I don't think we should hide information from the caller.

[Gavin Panella (allenap)]

On 6 August 2013 08:18, Raphaël Badin <email address hidden> wrote:
> You certainly have a point here… but, as Julian pointed out, I don't think we should hide information from the caller.

I'm in agreement now. We export the IsNotFoundError helper and I think
that's enough.

=== modified file 'management.go'
--- management.go	2013-07-23 21:51:46 +0000
+++ management.go	2013-08-05 12:52:47 +0000
@@ -255,6 +255,35 @@
     return nil
 }
 
+type ListRoleEndpointsRequest struct {
+    ServiceName    string
+    DeploymentName string
+    RoleName       string
+}
+
+// ListRoleEndpoints lists the open endpoints for the named service/deployment/role name.
+func (api *ManagementAPI) ListRoleEndpoints(request *ListRoleEndpointsRequest) ([]InputEndpoint, error) {
+    var err error
+    vmRole, err := api.GetRole(&GetRoleRequest{
+        ServiceName:    request.ServiceName,
+        DeploymentName: request.DeploymentName,
+        RoleName:       request.RoleName})
+
+    if err != nil {
+        return nil, err
+    }
+
+    for i, configSet := range vmRole.ConfigurationSets {
+        if configSet.ConfigurationSetType == CONFIG_SET_NETWORK {
+            endpointsP := vmRole.ConfigurationSets[i].InputEndpoints
+            if endpointsP != nil {
+                return *endpointsP, nil
+            }
+        }
+    }
+    return []InputEndpoint{}, nil
+}
+
 type AddRoleEndpointsRequest struct {
     ServiceName    string
     DeploymentName string

=== modified file 'management_test.go'
--- management_test.go	2013-07-22 17:30:34 +0000
+++ management_test.go	2013-08-05 12:52:47 +0000
@@ -844,6 +844,103 @@
     c.Check(err, ErrorMatches, "PUT request failed [(]500: Internal Server Error[)]")
 }
 
+type suiteListRoleEndpoints struct{}
+
+var _ = Suite(&suiteListRoleEndpoints{})
+
+func (suite *suiteListRoleEndpoints) TestWhenNoExistingEndpoints(c *C) {
+    var err error
+    existingRole := &PersistentVMRole{
+        ConfigurationSets: []ConfigurationSet{
+            {
+                ConfigurationSetType: CONFIG_SET_NETWORK,
+            },
+        },
+    }
+    responses := makeOKXMLResponse(c, existingRole)
+    record := []*X509Request{}
+    rigRecordingPreparedResponseDispatcher(&record, responses)
+    api := makeAPI(c)
+
+    request := &ListRoleEndpointsRequest{
+        ServiceName:    "foo",
+        DeploymentName: "foo",
+        RoleName:       "foo"}
+    endpoints, err := api.ListRoleEndpoints(request)
+
+    c.Assert(err, IsNil)
+    c.Assert(endpoints, DeepEquals, []InputEndpoint{})
+    c.Check(record, HasLen, 1)
+    // Check GetRole was performed.
+    assertGetRoleRequest(
+        c, api, record[0], request.ServiceName, request.DeploymentName,
+        request.RoleName)
+}
+
+func (suite *suiteListRoleEndpoints) TestWhenExistingEndpoints(c *C) {
+    var err error
+    endpoints := &[]InputEndpoint{
+        {
+            LocalPort: 123,
+            Name:      "test123",
+            Port:      1123,
+        },
+        {
+            LocalPort: 456,
+            Name:      "test456",
+            Port:      4456,
+        }}
+
+    existingRole := &PersistentVMRole{
+        ConfigurationSets: []ConfigurationSet{
+            {
+                ConfigurationSetType: CONFIG_SET_NETWORK,
+                InputEndpoints:       endpoints,
+            },
+        },
+    }
+    responses := makeOKXMLResponse(c, existingRole)
+    record := []*X509Request{}
+    rigRecordingPreparedResponseDispatcher(&record, responses)
+    api := makeAPI(c)
+
+    request := &ListRoleEndpointsRequest{
+        ServiceName:    "foo",
+        DeploymentName: "foo",
+        RoleName:       "foo"}
+    listedEndpoints, err := api.ListRoleEndpoints(request)
+
+    c.Assert(err, IsNil)
+    c.Assert(listedEndpoints, DeepEquals, *endpoints)
+    c.Check(record, HasLen, 1)
+    // Check GetRole was performed.
+    assertGetRoleRequest(
+        c, api, record[0], request.ServiceName, request.DeploymentName,
+        request.RoleName)
+}
+
+func (suite *suiteListRoleEndpoints) TestWhenGetRoleFails(c *C) {
+    responses := []DispatcherResponse{
+        // No role found.
+        {response: &x509Response{StatusCode: http.StatusNotFound}},
+    }
+    record := []*X509Request{}
+    rigRecordingPreparedResponseDispatcher(&record, responses)
+    api := makeAPI(c)
+
+    request := &ListRoleEndpointsRequest{
+        ServiceName:    "foo",
+        DeploymentName: "foo",
+        RoleName:       "foo"}
+    _, err := api.ListRoleEndpoints(request)
+
+    c.Check(err, ErrorMatches, "GET request failed [(]404: Not Found[)]")
+    c.Check(record, HasLen, 1)
+    assertGetRoleRequest(
+        c, api, record[0], request.ServiceName, request.DeploymentName,
+        request.RoleName)
+}
+
 type suiteAddRoleEndpoints struct{}
 
 var _ = Suite(&suiteAddRoleEndpoints{})