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
=== 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 @@
255 return nil255 return nil
256}256}
257257
258type ListRoleEndpointsRequest struct {
259 ServiceName string
260 DeploymentName string
261 RoleName string
262}
263
264// ListRoleEndpoints lists the open endpoints for the named service/deployment/role name.
265func (api *ManagementAPI) ListRoleEndpoints(request *ListRoleEndpointsRequest) ([]InputEndpoint, error) {
266 var err error
267 vmRole, err := api.GetRole(&GetRoleRequest{
268 ServiceName: request.ServiceName,
269 DeploymentName: request.DeploymentName,
270 RoleName: request.RoleName})
271
272 if err != nil {
273 return nil, err
274 }
275
276 for i, configSet := range vmRole.ConfigurationSets {
277 if configSet.ConfigurationSetType == CONFIG_SET_NETWORK {
278 endpointsP := vmRole.ConfigurationSets[i].InputEndpoints
279 if endpointsP != nil {
280 return *endpointsP, nil
281 }
282 }
283 }
284 return []InputEndpoint{}, nil
285}
286
258type AddRoleEndpointsRequest struct {287type AddRoleEndpointsRequest struct {
259 ServiceName string288 ServiceName string
260 DeploymentName string289 DeploymentName string
261290
=== 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 @@
844 c.Check(err, ErrorMatches, "PUT request failed [(]500: Internal Server Error[)]")844 c.Check(err, ErrorMatches, "PUT request failed [(]500: Internal Server Error[)]")
845}845}
846846
847type suiteListRoleEndpoints struct{}
848
849var _ = Suite(&suiteListRoleEndpoints{})
850
851func (suite *suiteListRoleEndpoints) TestWhenNoExistingEndpoints(c *C) {
852 var err error
853 existingRole := &PersistentVMRole{
854 ConfigurationSets: []ConfigurationSet{
855 {
856 ConfigurationSetType: CONFIG_SET_NETWORK,
857 },
858 },
859 }
860 responses := makeOKXMLResponse(c, existingRole)
861 record := []*X509Request{}
862 rigRecordingPreparedResponseDispatcher(&record, responses)
863 api := makeAPI(c)
864
865 request := &ListRoleEndpointsRequest{
866 ServiceName: "foo",
867 DeploymentName: "foo",
868 RoleName: "foo"}
869 endpoints, err := api.ListRoleEndpoints(request)
870
871 c.Assert(err, IsNil)
872 c.Assert(endpoints, DeepEquals, []InputEndpoint{})
873 c.Check(record, HasLen, 1)
874 // Check GetRole was performed.
875 assertGetRoleRequest(
876 c, api, record[0], request.ServiceName, request.DeploymentName,
877 request.RoleName)
878}
879
880func (suite *suiteListRoleEndpoints) TestWhenExistingEndpoints(c *C) {
881 var err error
882 endpoints := &[]InputEndpoint{
883 {
884 LocalPort: 123,
885 Name: "test123",
886 Port: 1123,
887 },
888 {
889 LocalPort: 456,
890 Name: "test456",
891 Port: 4456,
892 }}
893
894 existingRole := &PersistentVMRole{
895 ConfigurationSets: []ConfigurationSet{
896 {
897 ConfigurationSetType: CONFIG_SET_NETWORK,
898 InputEndpoints: endpoints,
899 },
900 },
901 }
902 responses := makeOKXMLResponse(c, existingRole)
903 record := []*X509Request{}
904 rigRecordingPreparedResponseDispatcher(&record, responses)
905 api := makeAPI(c)
906
907 request := &ListRoleEndpointsRequest{
908 ServiceName: "foo",
909 DeploymentName: "foo",
910 RoleName: "foo"}
911 listedEndpoints, err := api.ListRoleEndpoints(request)
912
913 c.Assert(err, IsNil)
914 c.Assert(listedEndpoints, DeepEquals, *endpoints)
915 c.Check(record, HasLen, 1)
916 // Check GetRole was performed.
917 assertGetRoleRequest(
918 c, api, record[0], request.ServiceName, request.DeploymentName,
919 request.RoleName)
920}
921
922func (suite *suiteListRoleEndpoints) TestWhenGetRoleFails(c *C) {
923 responses := []DispatcherResponse{
924 // No role found.
925 {response: &x509Response{StatusCode: http.StatusNotFound}},
926 }
927 record := []*X509Request{}
928 rigRecordingPreparedResponseDispatcher(&record, responses)
929 api := makeAPI(c)
930
931 request := &ListRoleEndpointsRequest{
932 ServiceName: "foo",
933 DeploymentName: "foo",
934 RoleName: "foo"}
935 _, err := api.ListRoleEndpoints(request)
936
937 c.Check(err, ErrorMatches, "GET request failed [(]404: Not Found[)]")
938 c.Check(record, HasLen, 1)
939 assertGetRoleRequest(
940 c, api, record[0], request.ServiceName, request.DeploymentName,
941 request.RoleName)
942}
943
847type suiteAddRoleEndpoints struct{}944type suiteAddRoleEndpoints struct{}
848945
849var _ = Suite(&suiteAddRoleEndpoints{})946var _ = Suite(&suiteAddRoleEndpoints{})

Subscribers

People subscribed via source and target branches

to all changes: