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

Revision history for this message
Julian Edwards (julian-edwards) wrote :

Approved, but please fix [1].

[0]
+// CompareInputEndpoints attempts to compare two InputEndpoint objects in a
+// way that's congruent with how Windows's Azure considers them. The name is
+// ignored, as is LoadBalancerProbe. The other fields are used for comparison,
+// except when LoadBalancedEndpointSetName is set, in which case LocalPort is
+// ignored.

>blink< !

I had not grokked any of that (but could be Lyme brain) from the docs. It's madness.

[1]

+ panic("wtf") // Safe to remove in Go 1.1

Should be OK to remove now, we are all supposed to be using Go 1.1 (add https://launchpad.net/~james-page/+archive/golang-backports) as it's the only supported version of Go.

[2]

+// Returns true to keep the endpoint defined in the role's configuration,
+// false to remove it. It is also welcome to mutate endpoints; they are passed
+// by reference.

By pointer, strictly speaking.

Very nice readable tests for the new comparison func, BTW!

review: Approve

« Back to merge proposal