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

Proposed by Raphaël Badin on 2013-08-05
Status: Merged
Approved by: Raphaël Badin on 2013-08-06
Approved revision: 212
Merged at revision: 212
Proposed branch: lp:~rvb/gwacl/list-endpoints
Merge into: lp:gwacl
Diff against target: 147 lines (+126/-0)
2 files modified
management.go (+29/-0)
management_test.go (+97/-0)
To merge this branch: bzr merge lp:~rvb/gwacl/list-endpoints
Reviewer Review Type Date Requested Status
Gavin Panella 2013-08-05 Approve on 2013-08-05
Review via email: mp+178552@code.launchpad.net

Commit message

Add ListRoleEndpoints method.

To post a comment you must log in.
Gavin Panella (allenap) wrote :

[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) wrote :

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) wrote :

> > 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) wrote :

> [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) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'management.go'
2--- management.go 2013-07-23 21:51:46 +0000
3+++ management.go 2013-08-05 12:52:47 +0000
4@@ -255,6 +255,35 @@
5 return nil
6 }
7
8+type ListRoleEndpointsRequest struct {
9+ ServiceName string
10+ DeploymentName string
11+ RoleName string
12+}
13+
14+// ListRoleEndpoints lists the open endpoints for the named service/deployment/role name.
15+func (api *ManagementAPI) ListRoleEndpoints(request *ListRoleEndpointsRequest) ([]InputEndpoint, error) {
16+ var err error
17+ vmRole, err := api.GetRole(&GetRoleRequest{
18+ ServiceName: request.ServiceName,
19+ DeploymentName: request.DeploymentName,
20+ RoleName: request.RoleName})
21+
22+ if err != nil {
23+ return nil, err
24+ }
25+
26+ for i, configSet := range vmRole.ConfigurationSets {
27+ if configSet.ConfigurationSetType == CONFIG_SET_NETWORK {
28+ endpointsP := vmRole.ConfigurationSets[i].InputEndpoints
29+ if endpointsP != nil {
30+ return *endpointsP, nil
31+ }
32+ }
33+ }
34+ return []InputEndpoint{}, nil
35+}
36+
37 type AddRoleEndpointsRequest struct {
38 ServiceName string
39 DeploymentName string
40
41=== modified file 'management_test.go'
42--- management_test.go 2013-07-22 17:30:34 +0000
43+++ management_test.go 2013-08-05 12:52:47 +0000
44@@ -844,6 +844,103 @@
45 c.Check(err, ErrorMatches, "PUT request failed [(]500: Internal Server Error[)]")
46 }
47
48+type suiteListRoleEndpoints struct{}
49+
50+var _ = Suite(&suiteListRoleEndpoints{})
51+
52+func (suite *suiteListRoleEndpoints) TestWhenNoExistingEndpoints(c *C) {
53+ var err error
54+ existingRole := &PersistentVMRole{
55+ ConfigurationSets: []ConfigurationSet{
56+ {
57+ ConfigurationSetType: CONFIG_SET_NETWORK,
58+ },
59+ },
60+ }
61+ responses := makeOKXMLResponse(c, existingRole)
62+ record := []*X509Request{}
63+ rigRecordingPreparedResponseDispatcher(&record, responses)
64+ api := makeAPI(c)
65+
66+ request := &ListRoleEndpointsRequest{
67+ ServiceName: "foo",
68+ DeploymentName: "foo",
69+ RoleName: "foo"}
70+ endpoints, err := api.ListRoleEndpoints(request)
71+
72+ c.Assert(err, IsNil)
73+ c.Assert(endpoints, DeepEquals, []InputEndpoint{})
74+ c.Check(record, HasLen, 1)
75+ // Check GetRole was performed.
76+ assertGetRoleRequest(
77+ c, api, record[0], request.ServiceName, request.DeploymentName,
78+ request.RoleName)
79+}
80+
81+func (suite *suiteListRoleEndpoints) TestWhenExistingEndpoints(c *C) {
82+ var err error
83+ endpoints := &[]InputEndpoint{
84+ {
85+ LocalPort: 123,
86+ Name: "test123",
87+ Port: 1123,
88+ },
89+ {
90+ LocalPort: 456,
91+ Name: "test456",
92+ Port: 4456,
93+ }}
94+
95+ existingRole := &PersistentVMRole{
96+ ConfigurationSets: []ConfigurationSet{
97+ {
98+ ConfigurationSetType: CONFIG_SET_NETWORK,
99+ InputEndpoints: endpoints,
100+ },
101+ },
102+ }
103+ responses := makeOKXMLResponse(c, existingRole)
104+ record := []*X509Request{}
105+ rigRecordingPreparedResponseDispatcher(&record, responses)
106+ api := makeAPI(c)
107+
108+ request := &ListRoleEndpointsRequest{
109+ ServiceName: "foo",
110+ DeploymentName: "foo",
111+ RoleName: "foo"}
112+ listedEndpoints, err := api.ListRoleEndpoints(request)
113+
114+ c.Assert(err, IsNil)
115+ c.Assert(listedEndpoints, DeepEquals, *endpoints)
116+ c.Check(record, HasLen, 1)
117+ // Check GetRole was performed.
118+ assertGetRoleRequest(
119+ c, api, record[0], request.ServiceName, request.DeploymentName,
120+ request.RoleName)
121+}
122+
123+func (suite *suiteListRoleEndpoints) TestWhenGetRoleFails(c *C) {
124+ responses := []DispatcherResponse{
125+ // No role found.
126+ {response: &x509Response{StatusCode: http.StatusNotFound}},
127+ }
128+ record := []*X509Request{}
129+ rigRecordingPreparedResponseDispatcher(&record, responses)
130+ api := makeAPI(c)
131+
132+ request := &ListRoleEndpointsRequest{
133+ ServiceName: "foo",
134+ DeploymentName: "foo",
135+ RoleName: "foo"}
136+ _, err := api.ListRoleEndpoints(request)
137+
138+ c.Check(err, ErrorMatches, "GET request failed [(]404: Not Found[)]")
139+ c.Check(record, HasLen, 1)
140+ assertGetRoleRequest(
141+ c, api, record[0], request.ServiceName, request.DeploymentName,
142+ request.RoleName)
143+}
144+
145 type suiteAddRoleEndpoints struct{}
146
147 var _ = Suite(&suiteAddRoleEndpoints{})

Subscribers

People subscribed via source and target branches

to all changes: