Merge lp://staging/~jtv/gwacl/service-endpoints into lp://staging/gwacl

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

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

I disagree. When a new region appears with "China" in its name, there's a good chance that it'll be in mainland China. Unless they called it "South China Sea" but it seems unlikely politically -- and "Indo-China" has gone right out of fashion. Conversely, there is a region in China that isn't in mainland China and isn't using the Chinese endpoints, and they took care not to name it after China.

So the name "China" seems to be a good indicator, and "locations with China in the names use the mainland-Chinese endpoints, the rest use the international ones" is a rule that accommodates both the known cases and expected future cases. It's explicit, just not as fragile and verbose as specifying known endpoints. It gives us a chance of supporting a new location without code changes.

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

Because it's the language's way of saying that you're not going to get your object modified by calling this. I don't care much about it either way.

> 62 + return prefixHost("blob", string(endpoint))
> 63 +}
>
> Given there's three types of storage, this should to be named
> BlobStorageAPI().

Fixed.

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

We do need that, but I felt that was in a different layer of responsibility. We already had a method for it, which I kept in place (although I made it re-use prefixHost so it's very simple).

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

This test verifies a specific relationship between the function's responses to different inputs. Not the actual responses themselves; those are covered by tests at the appropriate level and they're not relevant here. This is just a higher level of abstraction and I don't think that's a bad thi...

On Tuesday 06 Aug 2013 08:02:26 you wrote:
> I disagree. When a new region appears with "China" in its name, there's a

[snip]

Ok I just thought I'd bring it up. I think we'll end up getting bitten one way or
another but hey ... SEP.

> > 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)
>
> Because it's the language's way of saying that you're not going to get your
> object modified by calling this. I don't care much about it either way.

Ok - it's just that we know that non-pointer receivers don't meet the interface etc
etc. Most of the rest of the code uses pointer receivers.

> > 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.
>
> We do need that, but I felt that was in a different layer of responsibility.
> We already had a method for it, which I kept in place (although I made it
> re-use prefixHost so it's very simple).

I genuinely don't think it's a different layer of responsibility.

My reasoning is that each storage account is a different API endpoint. This code
says it returns an API endpoint, but it doesn't, it's not usable yet.

> This test verifies a specific relationship between the function's responses
> to different inputs. Not the actual responses themselves; those are
> covered by tests at the appropriate level and they're not relevant here.
> This is just a higher level of abstraction and I don't think that's a bad
> thing.
>
> Making the change you ask for would replace an explicit statement of the
> relationship with one that is implicit and easily lost in the unnecessary
> detail. But it also increases fragility. For example, we might at some
> point remove the trailing slashes after the URLs' hostnames. Completely
> arbitrary change, but simplestreams might force us to make it someday.
> That is completely outside the scope of this test: the test shouldn't care.
> But the way you're asking me to write it, it'd break this test for no
> reason. An engineer with a whole bunch of these failures on their hands
> would have to fix up the URLs in the tests on auto-pilot.
>
> On the other hand, imagine that the *relationship* that this test verifies
> breaks during development. The test would fail, because that's what it's
> for. But if I wrote it your way, the required change would be
> indistinguishable from the change required for the inconsequential change
> from the first example. What is the engineer's reasonable response? Fix
> up the URLs in the tests, on auto-pilot. At that point the test becomes
> meaningless -- because the relationship that's being tested was left
> implicit in the data instead of explicit, and because of the unnecessary
> fragility.

Ok we'll have to agree to disagree.

> > I don't like the "try to set this" stuff at all here. Let's just panic if
> > it's not set.
>
> Neither ...