Code review comment for lp:~jtv/gwacl/service-endpoints

Looks pretty good! I have some suggestions to make it better of course, but it
can land when you've made the changes.

27 + if strings.Contains(location, "China") {

It's "China East" and "China North" AFAICS, it might be worth being specific
instead of using Contains in case a new one pops up that's not actually in the
same domain. Although sod's law dictates whatever we do it'll be wrong ... but
I think explicit is always better as you end up with fewer surprises.

60 +// StorageAPI returns the base URL for the endpoint's storage API.
61 +func (endpoint APIEndpoint) StorageAPI() string {

Why not a pointer receiver? (same for ManagementAPI)

62 + return prefixHost("blob", string(endpoint))
63 +}

Given there's three types of storage, this should to be named BlobStorageAPI().
I'm not even sure the "API" part is accurate, but I'm not going to bikeshed over it.

Also, the storage URL returned here is still not complete, it would need the account prefixed.
It might be worth encapsulating that functionality in this object so that all the URL
manipulation is done in the same place. I'd have StorageAPI take an account name parameter.

114 +func (*endpointsSuite) TestGetEndpointMakesGoodGuessesForUknownRegions(c *C) {
115 + c.Check(
116 + GetEndpoint("South San Marino Highlands"),
117 + Equals,
118 + GetEndpoint("West US"))
119 + c.Check(
120 + GetEndpoint("Central China West"),
121 + Equals,
122 + GetEndpoint("China East"))
123 +}

I'm not sure I like this test at all. Its expected output is a function of the method being tested
rather than something either constant or independent of it, and that's kinda nasty. It also feels
fragile.

I'd much rather you were explicit in testing expected output to be the required endpoint URLs.

535 + // AzureEndpoint specifies a base service endpoint URL for the Azure APIs.
536 + // If this is not set, it will default to the international endpoint which
537 + // will not work in mainland China.
538 + //
539 + // Try to set this if at all possible. Use GetEndpoint() to obtain the
540 + // endpoint associated with a given service location, e.g. "West US" or
541 + // "North Europe" or "East China".
542 + AzureEndpoint APIEndpoint

I don't like the "try to set this" stuff at all here. Let's just panic if it's not set.
Out of interest, I wonder if Go works like C++ here and lets you supply a value that works with
the type's constructor as well as supplying the type itself.

558 + if endpoint == "" {
559 + // No API endpoint specified. Default to the international one.
560 + // This will not work for mainland China.
561 + endpoint = GetEndpoint("West US")

Remove this and panic() if it's not set.

563 + return prefixHost(context.Account, endpoint.StorageAPI())

And as I mentioned before, let's encapsulate this in APIEndpoint.

732 +// defaultManagement is the international management API for Azure.
733 +// (Mainland China gets a different URL).
734 +const defaultManagement = "https://management.core.windows.net/"

I hate Go for breaking the convention that consts are in CAPS. Grrr.

Anyway this should be defaultManagementURL, no? Sorry to bikeshed :)

895 - session, err := newX509Session(subscriptionID, "")
896 + session, err := newX509Session(subscriptionID, "", "West US")

Just picking one of these diff parts at random - but we should probably have a
makeRandomLocation() helper for tests where the location is irrelevant.