azure addressability changes

azure addressability changes

Implement Addresses()

https://code.launchpad.net/~natefinch/juju-core/005-azure-address/+merge/181117

(do not edit description out of merge proposal)

[natefinch, 2013-08-20 18:12:38]

Please take a look.

[jameinel, 2013-08-21 10:28:02]

the code and tests for Addresses looks sane to me, though it might be good to
have someone on Red squad confirm the actual APIs you are calling.

I have some questions about the Checker you implemented. The only bit I'm
genuinely worried about is mutating slices passed in by the caller. If you add a
test to reassure me we aren't changing them, the rest is all just suggestions.

https://codereview.appspot.com/13082044/diff/1/testing/checkers/checker.go
File testing/checkers/checker.go (right):

https://codereview.appspot.com/13082044/diff/1/testing/checkers/checker.go#ne...
testing/checkers/checker.go:157: // use it on very large slices
Is it hard to create a smart implementation that notices if the types are
hashable and can then just insert them into a map with counts and then compare
it?

counts := make(map[TYPE]int)
for i, v := range expected:
  counts[v] += 1
countsObtained := make(map[TYPE]int)
for i, v := range obtained:
  countsObtained[v] += 1

Obviously you need a bit more reflect magic than I've typed here, but you also
end up with nice summaries of the contents and what is different. (You could
even do it as a delta map and count up with expected, and down with obtained.)

If it is "cheap" to determine if something is hashable, I think the algorithm to
count them is nice and short and reasonably obvious. And probably most things
we're doing are strings, structs, and ints anyway.

https://codereview.appspot.com/13082044/diff/1/testing/checkers/checker.go#ne...
testing/checkers/checker.go:198: // that way we make sure the count of duplicate
items is the same
I'm a little concerned if removing them would mutate the caller's slices. (You
might compare to make sure they match, and then do other things with the slice.)

Can you confirm that it doesn't?

Even  better, add it as a test case so we ensure that we don't grow that
behavior accidentally in the future.

Another option that avoids rewriting the array N times would be to just allocate
a 'bool' array of the same length, and flag each entry with True once you've
seen it. When you iterate later, you can just check and 'continue' if that entry
has already been seen.

You still end up with N^2 comparisons, but you have a lot fewer reallocations.

My theoretical mind wonders if optimizing for the case of "lists are identical"
is worthwhile and starting the nested search at the offset of the outer search.
Or just doing one pass against all items until we have something that doesn't
match, and only then switch to the N^2 form. Certainly not terribly important
here.

https://codereview.appspot.com/13082044/diff/1/testing/checkers/checker_test.go
File testing/checkers/checker_test.go (right):

https://codereview.appspot.com/13082044/diff/1/testing/checkers/checker_test....
testing/checkers/checker_test.go:37: func (s *CheckerSuite) TestSameContents(c
*gc.C) {
I personally would rather see these split out as separate "Test*" cases, rather
than just commented sections. So a new suite for SameContents and then test
cases in that.

func (s *SameContentsSuite) TestSame(c *gc.C) {
  c.Check([]int(1, 2, 3), jc.SameContents, []int(1,2,3))
}

func (s *SameContentsSuite) TestEmpty(c *gc.C) {
  c.Check([]int{}, jc.SameContents, []int{})
}

etc.

That is a bit of a style thing, but I like having lots of very fast, very
focused test cases. (I do realize that you are using Check, so we will get
multiple errors in a single test case.)

As a taste thing, though, you're allowed to have a different opinion.

[natefinch, 2013-08-21 21:00:09]

Please take a look.

[natefinch, 2013-08-22 20:29:56]

Somehow my last message contents got dropped on the floor... not sure how that
happened.

The checker stuff was moved into a different branch, and the diffs now reflect
that (and I had implemented the changes john mentioned there).

This code was looked at by allenap, since I was having difficulty figuring out
how to mock out the server responses for the tests, so I'm pretty sure they're
good to go.

[jameinel, 2013-08-23 13:33:18]

LGTM as long as it has been tested live.

[jameinel, 2013-08-27 06:54:35]

On 2013/08/23 13:33:18, jameinel wrote:
> LGTM as long as it has been tested live.

You commented that this has been reviewed by others, but then didn't land this.
Is it blocked on something else?

[natefinch, 2013-09-04 15:14:28]

Please take a look.

[natefinch, 2013-09-04 15:41:58]

On 2013/09/04 15:14:28, nate.finch wrote:
> Please take a look.

Only change since the LGTM was a tiny tweak that I figured out was necessary in
live testing.  (and a merge from trunk)

[gz, 2013-09-04 16:00:08]

LGTM.

https://codereview.appspot.com/13082044/diff/18001/provider/azure/instance.go
File provider/azure/instance.go (right):

https://codereview.appspot.com/13082044/diff/18001/provider/azure/instance.go...
provider/azure/instance.go:64: err = azInstance.apiCall(false, func(c
*azureManagementContext) error {
I find this closure pretty hard to read, what with it being most of the outer
function and has differing return spelling.

https://codereview.appspot.com/13082044/diff/18001/provider/azure/instance_te...
File provider/azure/instance_test.go (right):

https://codereview.appspot.com/13082044/diff/18001/provider/azure/instance_te...
provider/azure/instance_test.go:174: instance.Address{
You don't actually need to repeat the type here.