Merge lp:~allenap/gwacl/remove-role-endpoints into lp:gwacl
Status: | Merged |
---|---|
Approved by: | Gavin Panella on 2013-07-25 |
Approved revision: | 218 |
Merged at revision: | 201 |
Proposed branch: | lp:~allenap/gwacl/remove-role-endpoints |
Merge into: | lp:gwacl |
Diff against target: |
994 lines (+554/-88) 8 files modified
example/management/run.go (+78/-47) management.go (+98/-3) management_base.go (+2/-2) management_base_test.go (+41/-0) management_test.go (+307/-7) x509dispatcher.go (+17/-23) xmlobjects.go (+7/-2) xmlobjects_test.go (+4/-4) |
To merge this branch: | bzr merge lp:~allenap/gwacl/remove-role-endpoints |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Julian Edwards (community) | Approve on 2013-07-23 | ||
Raphaël Badin (community) | 2013-07-19 | Approve on 2013-07-19 | |
Review via email:
|
Commit message
New method RemoveRoleEndpo
Julian Edwards (julian-edwards) wrote : | # |
On Friday 19 Jul 2013 13:52:26 Raphaël Badin wrote:
> 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?
We talked the other day of wrapping add/remove-
func that does just take simple port numbers.
I am still not sure what Azure considers to be the primary key for an
endpoint, the documentation is not clear enough.
- 195. By Gavin Panella on 2013-07-22
-
Fix typo.
Gavin Panella (allenap) wrote : | # |
> Looks good… but please have a look at [2].
>
> [0]
>
> The method is called 'RemoveRoleEndp
> 'RemoveRoleEndp
> 'RemoveRoleEndp
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.
> 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 LoadBalancedEnd
Protocol and VIP should be used for comparison.
- If LoadBalancedEnd
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 RemoveRoleEndpo
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?
Gavin Panella (allenap) wrote : | # |
> - If LoadBalancedEnd
> Protocol and VIP should be used for comparison.
... in *addition* to LoadBalancedEnd
- 196. By Gavin Panella on 2013-07-22
-
Switch to consts for LinuxProvisioni
ngConfiguration and NetworkConfigur ation. - 197. By Gavin Panella on 2013-07-22
-
Break out comparison of InputEndpoints into a separate function.
- 198. By Gavin Panella on 2013-07-22
-
Comment on RemoveRoleEndpoint.
- 199. By Gavin Panella on 2013-07-22
-
Redefine RemoveRoleEndpoints in terms of new method FilterRoleEndpo
ints. - 200. By Gavin Panella on 2013-07-22
-
Reorder.
- 201. By Gavin Panella on 2013-07-22
-
Document the filter function.
- 202. By Gavin Panella on 2013-07-22
-
Don't make the role filtering function public yet.
- 203. By Gavin Panella on 2013-07-22
-
Simplify filter definition.
- 204. By Gavin Panella on 2013-07-22
-
Explain why filterRoleEndpo
ints() is private. - 205. By Gavin Panella on 2013-07-22
-
Fix typo.
- 206. By Gavin Panella on 2013-07-22
-
Add use of {Add,Remove}
RoleEndpoints to the management example. - 207. By Gavin Panella on 2013-07-22
-
Fix typo.
Gavin Panella (allenap) wrote : | # |
All done, please take another look! Thanks.
Julian Edwards (julian-edwards) wrote : | # |
Approved, but please fix [1].
[0]
+// CompareInputEnd
+// 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 LoadBalancedEnd
+// 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:/
[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!
- 208. By Gavin Panella on 2013-07-23
-
Improve doc for CompareInputEnd
points.
Raphaël Badin (rvb) wrote : | # |
> > [2]
> >
> My thinking goes something like this:
>
> - Always ignore LoadBalancerProbe for the purposes of comparison.
>
> - If LoadBalancedEnd
> Protocol and VIP should be used for comparison.
>
> - If LoadBalancedEnd
> and VIP should be used for comparison.
Why omitting LocalPort in this case? I must say I'm a bit confused about what LoadBalancedEnd
>
> - 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 RemoveRoleEndpo
> 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.
Seems a bit overkill *if* we can figure out what the "natural key" of an endpoint is…
Raphaël Badin (rvb) wrote : | # |
> Seems a bit overkill *if* we can figure out what the "natural key" of an
> endpoint is…
I think (name), (protocol, local port) and (protocol, port) are natural keys for an endpoint.
Gavin Panella (allenap) wrote : | # |
On 23 July 2013 16:53, Raphaël Badin <email address hidden> wrote:
> I think (name), (protocol, local port) and (protocol, port) are
> natural keys for an endpoint.
So a comparison would go something like, in order:
- If Name matches, they're the same,
- If Protocol and LocalPort match, they're the same,
- If Protocol and Port match, they're the same,
- Otherwise they're not the same.
What about LoadBalancedEnd
Specifies a name for a set of load-balanced endpoints. Specifying
this element for a given endpoint adds it to the set.
That sounds like if it's set to a non-empty string then the natural
key would be (set-name, protocol, port, local-port).
This opens up more questions:
- For a load-balanced endpoint set, does the public port have to be
the same for all endpoints therein?
- What happens if a load-balanced set uses the same port as a
non-load-balanced endpoint? I assume they conflict. So, when
comparing endpoints we have to consider if they're the same
definition, or in conflict.
Also, I've seen some example XML for InputEndpoint that includes a Vip
tag (VIP in the GWACL structs). This ist shown in the docs for
GetRole, but not UpdateRole. I guess we can ignore this as Azure only
supports one VIP per deployment at present.
I'm still working on this branch... I think I'm going to land as soon
as I can with a whopping FIXME in CompareInputEnd
some experimentation to figure all the above out, at least that of it
which is relevant to us.
Gavin Panella (allenap) wrote : | # |
> [0]
...
> I had not grokked any of that (but could be Lyme brain) from the docs. It's
> madness.
Ha :) I've tried to improve the comment here. However, as you've
probably seen, the approach is still being discussed, so don't pay too
much attention to this yet.
>
> [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:/
> supported version of Go.
Done.
We can also upgrade the code underlying fork/http and fork/tls now;
it's based on 1.0.2 right now, for compatibility. Rapheël has already
tried this and it works fine, iirc, we just need to get round to doing
it.
>
> [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.
Is this a distinction that only makes a difference to a former C++
programmer, or is there a tangible difference in meaning?
>
> Very nice readable tests for the new comparison func, BTW!
Ta :) Very nice review :)
Gavin Panella (allenap) wrote : | # |
> > It may be worth having a FilterRoleEndpoints function, which behaves
> > like RemoveRoleEndpo
> > 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.
>
> Seems a bit overkill *if* we can figure out what the "natural key" of an
> endpoint is…
I did it, and it was a good move, imo. The filterRoleEndpoints
function is not public, and is only called by RemoveRoleEndpo
splitting up the function has made testing and reasoning easier.
Don't dismiss divide and conquer :)
- 209. By Gavin Panella on 2013-07-23
-
Merge trunk.
- 210. By Gavin Panella on 2013-07-23
-
Remove Go 1.0 compatibility hack.
Julian Edwards (julian-edwards) wrote : | # |
On 24/07/13 07:58, Gavin Panella wrote:
> We can also upgrade the code underlying fork/http and fork/tls now;
> it's based on 1.0.2 right now, for compatibility. Rapheël has already
> tried this and it works fine, iirc, we just need to get round to doing
> it.
Wicked. JFDI. :)
> Is this a distinction that only makes a difference to a former C++
> programmer, or is there a tangible difference in meaning?
Ah, ummm, maybe :)
Pointers are definitely different beasts to references in my head, but
Go blurs the distinction by eschewing the -> operator in favour of a .
for struct property access regardless of it being a pointer.
Does Go talk about references anywhere? I thought it just mentioned
pointer types.
>> Very nice readable tests for the new comparison func, BTW!
>
> Ta :) Very nice review :)
We're going to need to get a room at this rate.
Raphaël Badin (rvb) wrote : | # |
> On 23 July 2013 16:53, Raphaël Badin <email address hidden> wrote:
> > I think (name), (protocol, local port) and (protocol, port) are
> > natural keys for an endpoint.
>
> So a comparison would go something like, in order:
>
> - If Name matches, they're the same,
Why not stop here? The name is a mandatory (well, I think) identifier.
> I'm still working on this branch... I think I'm going to land as soon
> as I can with a whopping FIXME in CompareInputEnd
> some experimentation to figure all the above out, at least that of it
> which is relevant to us.
Yeah, we need this ASAP to do the work on the provider so it seems like a good idea to land this sooner rather than later.
Raphaël Badin (rvb) wrote : | # |
> On 23 July 2013 16:53, Raphaël Badin <email address hidden> wrote:
> > I think (name), (protocol, local port) and (protocol, port) are
> > natural keys for an endpoint.
>
> So a comparison would go something like, in order:
>
> - If Name matches, they're the same,
>
> - If Protocol and LocalPort match, they're the same,
>
> - If Protocol and Port match, they're the same,
>
> - Otherwise they're not the same.
On second thought that's probably a good solution. At first I thought we could only give it a list of names. That would be indeed much simpler but your solution is more flexible and will allow a user to remove endpoints based on the {port, protocol} and that's probably the best option.
> What about LoadBalancedEnd
>
> Specifies a name for a set of load-balanced endpoints. Specifying
> this element for a given endpoint adds it to the set.
>
> That sounds like if it's set to a non-empty string then the natural
> key would be (set-name, protocol, port, local-port).
>
> This opens up more questions:
>
> - For a load-balanced endpoint set, does the public port have to be
> the same for all endpoints therein?
AFAIUI this is meant to be used to load-balance things between machine and so I don't think it adds any meaning when it comes to deciding what the natural keys are… but this is all surmise of course because the documentation is — once again — not very clear.
- 211. By Gavin Panella on 2013-07-25
-
Change content type to application/xml.
- 212. By Gavin Panella on 2013-07-25
-
Improve logging experience for requests.
- 213. By Gavin Panella on 2013-07-25
-
Replace all fmt.Print* calls in the management example with log calls.
- 214. By Gavin Panella on 2013-07-25
-
Logging tweaks.
- 215. By Gavin Panella on 2013-07-25
-
Fix formatting.
- 216. By Gavin Panella on 2013-07-25
-
Fix the example endpoint so that it can be created.
- 217. By Gavin Panella on 2013-07-25
-
Couple of FIXMEs.
- 218. By Gavin Panella on 2013-07-25
-
UpdateRole needs to block until complete.
Gavin Panella (allenap) wrote : | # |
This _seems_ to work now. I'm landing it, and will fix the comparison algorithm separately.
Looks good… but please have a look at [2].
[0]
The method is called 'RemoveRoleEndp oints' but the parameter type is called 'RemoveRoleEndp ointRequest' , I think it should be 'RemoveRoleEndp oint*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 DeepEqual( endpoint, existingEndpoint) {
22 +}
…
40 + if reflect.
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?