Merge lp://staging/~julian-edwards/gwacl/availability-response into lp://staging/gwacl

Well done, well tested. Testing against bad XML in the response, and against bugs in your test helper, is particularly thorough. It's a shame that test setup is always such a pain in Go.

.

The interface for CheckHostedServiceNameAvailability makes perfect sense to me, but I suspect part of its documentation was written before it became clear to what extent the Azure API's version deserved to be hidden:

16 +// CheckHostedServiceNameAvailability returns true if the supplied name is
17 +// available to use as a cloud service name. It returns nil if it is available
18 +// or an error containing the reason if it is not.

The function now returns nil if the name can be used, not true as that first comment line says.

By the way, it would help to hide a bit of unjustified specificity in Azure's API here. The focus on availability is misleading. A user might well decide that the only sensible way to check for name availability is to allocate it, and ignore this function — or worse, remove a call to this function from client code. But we also need this function to detect failures from name-based blocking.

Maybe you could say "acceptable" instead of "available," and add something like "a name might be unacceptable because it is already in use, or because it contains a pattern that is rejected by Azure policy, such as a trademarked or a controversial word."

I half wonder whether AddHostedService should use the new function to help diagnose Azure errors with status codes 400 and 409, just so we can return decently specific error objects. But programmatically identifiable error objects are just not part of Go idiom — or at least, combining them with helpful error strings isn't.

.

In TestAddHostedServiceWithBadName, you do:

96 + c.Check(recordedRequests, HasLen, 1)

No need for that. It's built into checkOneRequest.

On 25/07/13 15:21, Jeroen T. Vermeulen wrote:
> Review: Approve
>
> Well done, well tested. Testing against bad XML in the response, and against bugs in your test helper, is particularly thorough. It's a shame that test setup is always such a pain in Go.
>
> .
>
> The interface for CheckHostedServiceNameAvailability makes perfect sense to me, but I suspect part of its documentation was written before it became clear to what extent the Azure API's version deserved to be hidden:
>
> 16 +// CheckHostedServiceNameAvailability returns true if the supplied name is
> 17 +// available to use as a cloud service name. It returns nil if it is available
> 18 +// or an error containing the reason if it is not.

Yes caught in my own headlights here. Fixed, thanks.

>
> The function now returns nil if the name can be used, not true as that first comment line says.
>
> By the way, it would help to hide a bit of unjustified specificity in Azure's API here. The focus on availability is misleading. A user might well decide that the only sensible way to check for name availability is to allocate it, and ignore this function — or worse, remove a call to this function from client code. But we also need this function to detect failures from name-based blocking.
>
> Maybe you could say "acceptable" instead of "available," and add something like "a name might be unacceptable because it is already in use, or because it contains a pattern that is rejected by Azure policy, such as a trademarked or a controversial word."
>
> I half wonder whether AddHostedService should use the new function to help diagnose Azure errors with status codes 400 and 409, just so we can return decently specific error objects. But programmatically identifiable error objects are just not part of Go idiom — or at least, combining them with helpful error strings isn't.

Possibly, but I would much rather make random IDs UUIDs instead.

>
> .
>
> In TestAddHostedServiceWithBadName, you do:
>
> 96 + c.Check(recordedRequests, HasLen, 1)
>
> No need for that. It's built into checkOneRequest.
>

Ta.