Code review comment for lp://staging/~dpb/charms/precise/haproxy/fix-service-entries

Thanks a lot for the review, Sidnei. Here is my response:

On Tue, Jan 21, 2014 at 5:10 AM, Sidnei da Silva <
<email address hidden>> wrote:

> Review: Needs Fixing
>
> Hi David, thanks for the branch. A few comments below.
>
> 43 + service = {}
> 44 + # First come, first serve, make sure all options other than
> 45 + # "servers" are represented in the combined dict
> 46 + for key in old_service.keys():
> 47 + service[key] = old_service[key]
>
> This can be rewritten as:
>
> service = old_service.copy()
>
>
Done, and further optimized with a copy() and update() in the right order.
 I think it is much more readable now.

> 56 + service["servers"] = old_servers + new_servers
>
> This doesn't account for duplicate server entries. I would deduplicate on
> (ip, port). The trick is if options differ for two definitions of the same
> (ip, port) combination, what to do so it's deterministic. Maybe sort on the
> whole tuple contents and pick first?
>

After our discussion in IRC, we decided to removed strict duplicates only
(since ip/port duplication is supported), but strict duplicates are likely
a programming error or a configuration error by the admin/charm. I have
updated this code as well.

There are test cases for everything I added, please re-review when you get
a chance.

Thanks!

--
David Britton <email address hidden>