Code review comment for lp://staging/~julian-edwards/gwacl/xml-definitions

Please consider the suggestions I have; apart from what I point out in [7] nothing is really *wrong* but I think we can improve things a bit:

[0]

Please use 'gofmt' to reformat the code. The structures with their "xml decorations" will be much more readable.

[1]

21 +func makeEndpoint(params endpointParams) *InputEndpoint {
[...]
40 +func makeLinuxProvisioningConfiguration(params provconfParams) *LinuxProvisioningConfiguration {
etc.

It's a detail but in Go, creator methods like this tend to use the 'New' prefix (instead of make); the typical declaration is: "NewT() *T". I know it does not matter much but it's good to follow the Go standards whenever it does not hurt.

[2]

149 +// It's impossible to have any kind of common method inherited into all the
150 +// various serializable objects because the receiver of the method is the
151 +// wrong class which confuses the xml marshaller. Hence, this mess.
152 +type AzureObject interface {
153 + Serialize() (string, error)
154 +}
155 +
156 +func toxml(obj AzureObject) (string, error) {
157 + out, err := xml.MarshalIndent(obj, "", " ")
158 + if err != nil {
159 + return "", err
160 + }
161 + return string(out), nil
162 +}
[...]
183 +func (c *InputEndpoint) Serialize() (string, error) {
184 + return toxml(c)
185 +}

You said you wanted to do this in order to have a Serialize() method on each object instead of having to call a global method. I agree with you in principle but here that it would be better. However, in Go, what you had to do in order to do this is add some boilerplate code that simply limits our ability to control how the xml is produced (by wrapping it in a method that does basically nothing but call xml.MarshalIndent()). I'll leave it up to you because you seemed to have very strong feelings about this but I'd advise getting rid of this completely and simply using xml.MarshalIndent() in the tests.

[3]

259 + c.Check(xml, Equals, fmt.Sprintf(`
260 +<LinuxProvisioningConfiguration>
261 + <ConfigurationSetType>LinuxProvisioningConfiguration</ConfigurationSetType>

If Gavin can write a Dedent() method, the definition of the expected XML in the tests will look much better.

[4]

205 +type Role struct {
206 + RoleSize string `xml:"RoleSize"`
207 + RoleName string `xml:"RoleName"`
208 + RoleType string `xml:"RoleType"` // Always "PersistentVMRole"
209 + ConfigurationSet []LinuxProvisioningConfiguration `xml:"ConfigurationSets>ConfigurationSet"`
210 +}

There is no support for default values of a field in a structure and this is a PITA. But I suggest creating a method with the following signature: "NewRole(roleSize string, roleName string, configurationSet []LinuxProvisioningConfiguration)" that will initialize the new struct's 'roleType' field with "PersistentVMRole".

Same for the other structures that have constant fields. I know that creating this code, for python programmers, seems like wasted time but AFAIK this is the only way to do this in Go and this way, you encapsulate that knowledge in the code and can test for it, instead of expecting the caller to do the right thing based on a comment in the code.

[5]

134 func init() {
135 // Staggeringly, rand will produce the same numbers from the start of
136 // program invocation unless you seed it ...

Not something you've changed in this branch but just a remark: this is actually well documented:
"Seed uses the provided seed value to initialize the generator to a deterministic state. If Seed is not called, the generator behaves as if seeded by Seed(1)."
http://golang.org/pkg/math/rand/#Seed
… so this is not so "staggering" ;)

[6]

254 +func (suite *GwaclSuite) TestLinuxProvisioningConfiguration(c *C) {

I see you're using one test suite for all the tests in this package. When Jeroen and I were working on gomaasapi, we were told it was better to use a new test suite for each "submodule" (i.e. file in the module) (and that's what we did in gomaasapi); the main reason being that it makes it more easy to refactor things in particular when you've moving things around. Also, it offers more flexibility when you want to run the tests from one file and one file only, which can come in handy when a module becomes huge.

[7]

8 +func MakeRandomBool() bool {
9 + v := rand.Intn(1)
10 + if v == 0 {return false}
11 + return true
12 +}

This will always return false because the boundary passed to rand.Intn() is non-inclusive.
You need to use rand.Intn(2) here.
"Intn returns, as an int, a non-negative pseudo-random number in [0,n). It panics if n <= 0."
http://golang.org/pkg/math/rand/#Intn

[8]

36 +type provconfParams struct {
37 + ConfigurationSetType, hostname, username, password string
38 + disableSshPasswordAuthentication string}
[...]
48 + if params.disableSshPasswordAuthentication == "" {
49 + disable_ssh = MakeRandomBool()
50 + } else {
51 + if params.disableSshPasswordAuthentication == "true" {
52 + disable_ssh = true
53 + } else {
54 + disable_ssh = false
55 + }
56 + }

Why is 'disableSshPasswordAuthentication' a string in 'provconfParams' and 'DisableSshPasswordAuthentication' is a bool in 'LinuxProvisioningConfiguration'. Why not make disableSshPasswordAuthentication a bool in provconfParams? Wouldn't it avoid the bizarre dance in makeLinuxProvisioningConfiguration (lines 48-56 of the diff)?

[9]

67 +func makeOSVirtualHardDisk(params osVirtualHardDiskParams) *OSVirtualHardDisk {
68 + if params.HostCaching == "" {
69 + if MakeRandomBool() {
70 + params.HostCaching = "ReadOnly"
71 + } else {
72 + params.HostCaching = "ReadWrite"
73 + }
74 + }

I'm concerned that such an important information (that HostCaching should be "ReadOnly" or "ReadWrite") is buried in the test_helper.go file.

How about defining enums as best as possible given Go's limitations:

type HostCachingType string

const (
    HostCachingRO HostCachingType = "ReadOnly"
    HostCachingRW HostCachingType = "ReadWrite"
)

and then we would define "NewOSVirtualHardDisk(hostCaching HostCachingType, …)" (same as what I suggest in [4])