Treat tenancy as non enterprise

[williammartin]

Description

I think there's some things I'd like to move from https://github.com/cli/cli/blob/24481c5c2e49d3529a6621953b8878fe0514ff22/internal/ghinstance/host.go into go-gh but I think having this behaviour as an obvious standalone change is valuable without muddying the waters.

[andyfeller]

Would we want to bring over the isTenancy() tests over too?

[andyfeller]

Any concerns about cli/go-gh not 1) normalizing the hostname as part of the boolean checks or 2) extracting the tenant name from a host name like cli/cli?

// IsEnterprise reports whether a non-normalized host name looks like a GHE instance.
func IsEnterprise(h string) bool {
	normalizedHostName := NormalizeHostname(h)
	return normalizedHostName != defaultHostname && normalizedHostName != localhost
}

// IsTenancy reports whether a non-normalized host name looks like a tenancy instance.
func IsTenancy(h string) bool {
	normalizedHostName := NormalizeHostname(h)
	return strings.HasSuffix(normalizedHostName, "."+tenancyHost)
}

// TenantName extracts the tenant name from tenancy host name and
// reports whether it found the tenant name.
func TenantName(h string) (string, bool) {
	normalizedHostName := NormalizeHostname(h)
	return cutSuffix(normalizedHostName, "."+tenancyHost)
}

func isGarage(h string) bool {
	return strings.EqualFold(h, "garage.github.com")
}

// NormalizeHostname returns the canonical host name of a GitHub instance.
func NormalizeHostname(h string) string {
	hostname := strings.ToLower(h)
	if strings.HasSuffix(hostname, "."+defaultHostname) {
		return defaultHostname
	}
	if strings.HasSuffix(hostname, "."+localhost) {
		return localhost
	}
	if before, found := cutSuffix(hostname, "."+tenancyHost); found {
		idx := strings.LastIndex(before, ".")
		return fmt.Sprintf("%s.%s", before[idx+1:], tenancyHost)
	}
	return hostname
}

[williammartin]

Any concerns about cli/go-gh not 1) normalizing the hostname as part of the boolean checks

Gah, I don't know. Some normalization does happen in go-gh so maybe? I just don't have a clear picture of when this happens and why it is important. It's probably best to match behaviour though even if we don't fully understand.

go-gh/pkg/auth/auth.go

Lines 155 to 164 in d88d88f

	func normalizeHostname(host string) string {
	hostname := strings.ToLower(host)
	if strings.HasSuffix(hostname, "."+github) {
	return github
	}
	if strings.HasSuffix(hostname, "."+localhost) {
	return localhost
	}
	return hostname
	}

2. extracting the tenant name from a host name like cli/cli?

Don't see any reason to have this behaviour, it's used for something to do with repo remote URLs and SSH (though I do wonder if the same thing should be happening for gists, that's a separate issue.

[williammartin]

Would we want to bring over the isTenancy() tests over too?

Not specifically but this reminded me that I definitely want to add tests to

go-gh/pkg/auth/auth_test.go

Line 10 in d88d88f

func TestTokenForHost(t *testing.T) {

Really silly of me to forget.

[williammartin]

Strictly speaking we shouldn't need this backport anymore but I'd rather change cli/cli and this at the same time, and I don't want to change cli/cli right now.

[andyfeller]

Would we want to bring over the isTenancy() tests over too?

Not specifically but this reminded me that I definitely want to add tests to

go-gh/pkg/auth/auth_test.go

Line 10 in d88d88f

func TestTokenForHost(t *testing.T) {

Really silly of me to forget.

I know, but I hate to say we have a test suite for IsEnterprise already:

go-gh/pkg/auth/auth_test.go

Lines 229 to 258 in ee82f3a

	func TestIsEnterprise(t *testing.T) {
	tests := []struct {
	name string
	host string
	wantOut bool
	}{
	{
	name: "github",
	host: "github.com",
	wantOut: false,
	},
	{
	name: "localhost",
	host: "github.localhost",
	wantOut: false,
	},
	{
	name: "enterprise",
	host: "mygithub.com",
	wantOut: true,
	},
	}
	for _, tt := range tests {
	t.Run(tt.name, func(t *testing.T) {
	out := isEnterprise(tt.host)
	assert.Equal(t, tt.wantOut, out)
	})
	}
	}

You really sure?

[andyfeller]

Looks good all in all.

I'm going to defer to you if you really don't think bringing over the tests for isTenancy is necessary here.

[williammartin]

Bleh. So firstly, I have to say that the existing tests are already too knowledgable about implementation details. That the public function TokenForHost is totally untested is an indictment on the current state of these tests.

TokenForHost is what users of the go-gh module rely on. That's the function that they expect to work. Testing the private functions that support TokenForHost instead not only reduces our ability to avoid regressions but actually congeals the existing implementation, increasing maintenance costs.

I'd added cases to the private tokenForHost test try and get as much of a black box test as possible within the current framework. At your prompting I've also added a test to communicate intent around how isEnterprise should handle tenants. I don't think there's much value in adding tests around isTenant.

I know, but I hate to say we have a test suite for IsEnterprise already

Yeh and I don't think it's good. What I really want to do is rework all the tests to avoid them knowing about any private functions but I don't have the appetite to tackle that here.

---

All that said, I really appreciate you pushing on the tests angle 😊