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

> 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'

Good spot, fixed.

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

Good point! I'll do that before I land.

>
> [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?

My thinking goes something like this:

- Always ignore LoadBalancerProbe for the purposes of comparison.

- If LoadBalancedEndpointSetName is defined, then LocalPort, Port,
  Protocol and VIP should be used for comparison.

- If LoadBalancedEndpointSetName is *not* defined, then Port, Protocol
  and VIP should be used for comparison.

- This leaves Name. I'm not sure what purpose this serves, other than
  as a pretty name for the UI. If so, it's irrelevant for the purposes
  of comparison.

It may be worth having a FilterRoleEndpoints function, which behaves
like RemoveRoleEndpoints, but accepts a func(*InputEndpoint) bool
function which can either mutate the endpoint or remove it (by
returning false). I think this might better support the desired
behaviour in juju-core, and make us more flexible to change.

What do you think?