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

Thanks for looking at the branch Raphers.

On 11/03/13 21:37, Raphaël Badin wrote:
> 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.

I've done this. It's screwed up other formatting but I am beyond caring.

>
> [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.

Hmmm. I wanted to make a test factory-only method. Our convention for
those is to have makeXXX. In particular, I do not want to export these
methods for outside use at all, since they generate random values.

> [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.

I genuinely don't understand what your suggestion buys us. Feel free to
convince me more, but I want to see concrete advantages. As it stands,
the way it's done encapsulates some formatting (IME this is *extremely*
important for debugging at a later date) and stringifies the output from
Marshal[Indent] because by default it gives you a byte array (and I
really don't understand how that is useful and why they did it like that).

> does basically nothing but call xml.MarshalIndent()

It also stringifies it.

> limits our ability to control how the xml is produced

It doesn't limit anything. You're free to reformat the xml if you're
crazy enough :)

In addition, having a single point through which stuff is converted to
xml means we can change it in one place and it affects all the objects,
keeping them consistent.

> [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.

Indeed! Failing that, I'll cave in and do continuation strings. But I
hate them for long output.

> [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.

I think this is a good suggestion, I'll do it.

>
> [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" ;)

It's staggering insofar as it's the first language I've used that
doesn't preseed the random number generator. It produces unexpected
default results, and that is asinine IMO, especially since every program
that uses random numbers will have the same boilerplate code.

>
> [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.

Yeah I wondered about that. I was too lazy to fix it. I'll change it
in a followup branch.

>
> [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

Most people would think that 0,n would produce 0 or 1 :/ Also, is that
a doc typo with [ vs ) ?

>
> [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)?

Because how else can you have a default value that is not one of true or
false?

In Python we're very used to passing a default value of None when a
parameter is a bool. You can't do this in Go.

>
> [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 strinest 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])
>

Good call, I'd done that (although the language is too stupid to
auto-convert between the const type and a string so I added casts.

I'm going to land it now. If there's anything else to change we can do
it in a followup branch.

Cheers.