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


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

Neither do I.  See the merge proposal.


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

I don't know what you mean.


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

Separate branch.  See the merge proposal.  Expect collateral damage.


> 563     +    return prefixHost(context.Account, endpoint.StorageAPI())
> 
> And as I mentioned before, let's encapsulate this in APIEndpoint.

Hmm... I tried doing that just now, but it made things appreciably worse, because of the way it breaks down separation of responsibilities.  Instead, I documented the need to prefix the account name.


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

Yes, but it's incredibly long -- and this variable just became a lot less significant so I felt I could cheat a bit.


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

Yes.  The tragedy is that it makes the code even longer, for something irrelevant.  I can do this in a separate branch if you like; the diff is already quite big and I'd like to get it landed before conflicts emerge.