Merge lp://staging/~julian-edwards/gwacl/affinity-group into lp://staging/gwacl

Looks good!

[0]

59 + var err error
60 + url := "affinitygroups/" + request.Name

and

78 + var err error
79 + url := "affinitygroups/" + request.Name

You need to use checkPathComponents() to make sure request.Name can be safely used in the url.

[1]

279 +}
280 func (suite *xmlSuite) TestDeployment(c *C) {

Missing an empty line here I think… did you run 'make format'?

[2]

You might want to do this in another branch or in here since it's really tiny but anyway, I'm mentioning it here so that we don't forget: NewCreateHostedServiceWithLocation() needs to be updated to take an affinity group name (AffinityGroup is already a field in the CreateHostedService struct). Then we need to update the call to NewCreateHostedServiceWithLocation() in example/management/run.go to take the name of the newly created affinity group. In this branch as it is now, you're creating the affinity group in example/management/run.go but the created hosted service is not linked to it.

[3]

36 +type CreateAffinityGroupRequest struct {
37 + CreateAffinityGroup *CreateAffinityGroup
38 +}

Not sure if it's really useful but I guess it's more coherent with the other calls to have this intermediate CreateAffinityGroupRequest structure…?

[4]

9 + affinityGroupName := gwacl.MakeRandomHostname("affinitygroup")

Why did you use MakeRandomHostname() here? AFAIK this is not a hostname is it? Hum, looking at names.go we might want to expose makeRandomIdentifier() and use that directly.

[5]

165 +//
166 +type CreateAffinityGroup struct {

Maybe include the link to the API documentation in the "docstring"…? Here and in a couple of other places.

Thank you for the review.

On 10/07/13 18:16, Raphaël Badin wrote:
> Review: Approve
>
> Looks good!
>
> [0]
>
> 59 + var err error
> 60 + url := "affinitygroups/" + request.Name
>
> and
>
> 78 + var err error
> 79 + url := "affinitygroups/" + request.Name
>
> You need to use checkPathComponents() to make sure request.Name can be safely used in the url.

Gnargh, I forgot. Again.

There has to be a better way of dealing with this.

>
> [1]
>
> 279 +}
> 280 func (suite *xmlSuite) TestDeployment(c *C) {
>
> Missing an empty line here I think… did you run 'make format'?

I did - maybe it's a merge error...

>
> [2]
>
> You might want to do this in another branch or in here since it's really tiny but anyway, I'm mentioning it here so that we don't forget: NewCreateHostedServiceWithLocation() needs to be updated to take an affinity group name (AffinityGroup is already a field in the CreateHostedService struct). Then we need to update the call to NewCreateHostedServiceWithLocation() in example/management/run.go to take the name of the newly created affinity group. In this branch as it is now, you're creating the affinity group in example/management/run.go but the created hosted service is not linked to it.

Separate branch, yes :)

>
> [3]
>
> 36 +type CreateAffinityGroupRequest struct {
> 37 + CreateAffinityGroup *CreateAffinityGroup
> 38 +}
>
> Not sure if it's really useful but I guess it's more coherent with the other calls to have this intermediate CreateAffinityGroupRequest structure…?

Consistency always wins. Besides, we may want to add more parameters
later as previously discussed.

>
> [4]
>
> 9 + affinityGroupName := gwacl.MakeRandomHostname("affinitygroup")
>
> Why did you use MakeRandomHostname() here? AFAIK this is not a hostname is it? Hum, looking at names.go we might want to expose makeRandomIdentifier() and use that directly.

Because it does exactly what I need. We can rename it as necessary I
suppose.

>
> [5]
>
> 165 +//
> 166 +type CreateAffinityGroup struct {
>
> Maybe include the link to the API documentation in the "docstring"…? Here and in a couple of other places.
>

Yes - despite me telling others to do the same thing, and mostly
remembering to do so myself, I always miss one!

Cheers.

> [2]
>
> You might want to do this in another branch or in here since it's
> really tiny but anyway, I'm mentioning it here so that we don't
> forget: NewCreateHostedServiceWithLocation() needs to be updated to
> take an affinity group name (AffinityGroup is already a field in the
> CreateHostedService struct). Then we need to update the call to
> NewCreateHostedServiceWithLocation() in example/management/run.go to
> take the name of the newly created affinity group. In this branch
> as it is now, you're creating the affinity group in
> example/management/run.go but the created hosted service is not
> linked to it.

Perhaps we ought to have a NewCreateHostedServiceWithLocationParams
struct so that we can pass optional arguments into the constructor?
</black-humour>

I'm coming to the opinion that the NewBlah() pattern for constructors
is actually an antipattern. Calling conventions for Go's functions
have an impedance mismatch with constructing a struct directly, so it
always feels like hammering a square peg into the proverbial.

I'm wondering if something like the following might be better:

  type Thing struct {
      ...
  }

  func (t *Thing) Init() *Thing {
      // Initialise, e.g. set default fields if they're nil.
      ...
      return t
  }

  var thing *Thing = &Thing{...}.Init()

This still supports multiple constructors, any of which /could/ also
take args. The struct itself has to be exported for use by other
packages, which may be an undesirable side-effect in some cases.

Attempt to merge into lp:gwacl failed due to conflicts:

text conflict in management_base.go
text conflict in management_base_test.go
text conflict in xmlobjects_test.go

> [2]
>
> You might want to do this in another branch or in here since it's
> really tiny but anyway, I'm mentioning it here so that we don't
> forget: NewCreateHostedServiceWithLocation() needs to be updated to
> take an affinity group name (AffinityGroup is already a field in the
> CreateHostedService struct).  Then we need to update the call to
> NewCreateHostedServiceWithLocation() in example/management/run.go to
> take the name of the newly created affinity group.  In this branch
> as it is now, you're creating the affinity group in
> example/management/run.go but the created hosted service is not
> linked to it.

Perhaps we ought to have a NewCreateHostedServiceWithLocationParams
struct so that we can pass optional arguments into the constructor?
</black-humour>

I'm coming to the opinion that the NewBlah() pattern for constructors
is actually an antipattern. Calling conventions for Go's functions
have an impedance mismatch with constructing a struct directly, so it
always feels like hammering a square peg into the proverbial.

I'm wondering if something like the following might be better:

  type Thing struct {
      ...
  }

  func (t *Thing) Init() *Thing {
      // Initialise, e.g. set default fields if they're nil.
      ...
      return t
  }

  var thing *Thing = &Thing{...}.Init()

This still supports multiple constructors, any of which /could/ also
take args. The struct itself has to be exported for use by other
packages, which may be an undesirable side-effect in some cases.

I too have felt uneasy about some of these "constructors" as it's just
as easy to set the fields directly.

The only time it's particularly useful is when you have the need for a
non-Go default value (like our XLMNS).