Code review comment for lp:~dimitern/juju-core/382-api-provisioner-machine-nics

Please take a look.

https://codereview.appspot.com/84570043/diff/1/[revision%20details]
File [revision details] (right):

https://codereview.appspot.com/84570043/diff/1/[revision%20details]#newcode1
[revision details]:1: Old revision:
<email address hidden>
> LGTM. I am a little panicked around future-proofing though, I
> think these api structs will want to look a little different,
> or maybe quite a bit different, when we add other clouds and
> better networking support. We seem to be reserving some
> pretty generic API names 'AddNetworks" and will probably want
> a number of revisions. We may get to several -Ex suffixes or
> something.

Structs can evolve and we can phase-out API calls and add new ones with
extended functionality. This is a general problem with the API w/o
versioning.

https://codereview.appspot.com/84570043/diff/1/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/84570043/diff/1/state/api/params/internal.go#newcode398
state/api/params/internal.go:398: NetworkName string
On 2014/04/04 15:40:02, gz wrote:
> This will probably be a non-name id for clouds other than MAAS.
Shouldn't matter
> apart from naming.

We can add fields as needed later (and "name" can also be a UUID if
needed :)

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/machine.go
File state/api/provisioner/machine.go (right):

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/machine.go#newcode85
state/api/provisioner/machine.go:85: if interfaces[i].MachineTag !=
m.tag {
On 2014/04/04 15:40:02, gz wrote:
> Why the conditional?

To make sure we can only add NICs to the current machine (in case it's
empty or different). There are tests for that in state/api/provisioner.

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/provisioner_test.go
File state/api/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/provisioner_test.go#newcode359
state/api/provisioner/provisioner_test.go:359: c.Assert(net.Name(),
gc.Equals, expectParams.Name)
On 2014/04/04 15:40:02, gz wrote:
> Can use c.Check for these two after the err Assert.

Done.

https://codereview.appspot.com/84570043/diff/1/state/api/provisioner/provisioner_test.go#newcode412
state/api/provisioner/provisioner_test.go:412: // Ensure only the first
error is reported.
On 2014/04/04 15:40:02, gz wrote:
> This feels like a seperate testcase really, the current test is > one
screen
> high which is a bit of a personal bugbear of mine. Sharing setup steps
is not
> that hard.

Moved to a separate test case.

https://codereview.appspot.com/84570043/diff/1/state/apiserver/provisioner/provisioner_test.go
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/84570043/diff/1/state/apiserver/provisioner/provisioner_test.go#newcode828
state/apiserver/provisioner/provisioner_test.go:828:
c.Assert(net.Name(), gc.Equals, "vlan42")
On 2014/04/04 15:40:02, gz wrote:
> Can use c.Check, and below.

Done.

https://codereview.appspot.com/84570043/diff/1/state/apiserver/provisioner/provisioner_test.go#newcode839
state/apiserver/provisioner/provisioner_test.go:839: // Test it fails
with a non-environment-manager.
On 2014/04/04 15:40:02, gz wrote:
> This feels like a seperate testcase.

Done.

https://codereview.appspot.com/84570043/diff/1/state/apiserver/provisioner/provisioner_test.go#newcode850
state/apiserver/provisioner/provisioner_test.go:850: func (s
*withoutStateServerSuite) TestAddNetworkInterface(c *gc.C) {
On 2014/04/04 15:40:02, gz wrote:
> Giant complicated test again...

I did some more refactoring to shorten the test and reuse common bits.
There are a lot of cases to test and the setup is more or less the same.

https://codereview.appspot.com/84570043/