Code review comment for lp:~allenap/gwacl/remove-role-endpoints

Raphaël Badin (rvb) wrote :

Looks good… but please have a look at [2].

[0]

The method is called 'RemoveRoleEndpoints' but the parameter type is called 'RemoveRoleEndpointRequest', I think it should be 'RemoveRoleEndpoint*s*Request'

[1]

As always, I'd like to see this method exercised in the real world (i.e. in the example script).

[2]

21 + InputEndpoints []InputEndpoint
22 +}

40 + if reflect.DeepEqual(endpoint, existingEndpoint) {

This is clearly okay for the provider but I wonder if it would not be more straightforward, instead of passing whole endpoints objects, to pass just a list of the ports (the external ports) corresponding to the endpoints to be removed… because the code we have here imposes to create InputEndpoints structures where only the "natural key" (i.e. the external port) could be sufficient to identify the endpoints to be deleted. What do you think?

review: Approve

« Back to merge proposal