Treat tenancy as non enterprise by williammartin · Pull Request #151 · cli/go-gh

williammartin · 2024-02-26T13:27:38Z

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 · 2024-02-26T13:38:33Z

+// TenancyHost is the domain name of a tenancy GitHub instance.
+const tenancyHost = "ghe.com"
+
 func isEnterprise(host string) bool {
-	return host != github && host != localhost
+	return host != github && host != localhost && !isTenancy(host)
+}
+
+func isTenancy(host string) bool {
+	return strings.HasSuffix(host, "."+tenancyHost)
 }


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 }

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

}

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 · 2024-02-26T13:42:30Z

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 · 2024-02-28T13:30:36Z

 	return hostname
 }
+
+// Backport strings.CutSuffix from Go 1.20.


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 · 2024-02-28T13:44:53Z

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 · 2024-02-28T14:06:14Z

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 😊

Following on the heels of #151, this commit exports the internal logic used to determine if a host is GHES deployment or a tenant. Exporting these functions allow extension authors like `github/gh-copilot` to provide tailored experiences consistently with `gh`.

andyfeller reviewed Feb 26, 2024

View reviewed changes

williammartin force-pushed the wm/treat-tenancy-as-non-enterprise branch 2 times, most recently from acdb31f to 2054b1b Compare February 26, 2024 14:08

williammartin commented Feb 28, 2024

View reviewed changes

Treat tenancy as non enterprise

ee82f3a

williammartin force-pushed the wm/treat-tenancy-as-non-enterprise branch from 2054b1b to ee82f3a Compare February 28, 2024 13:34

andyfeller approved these changes Feb 28, 2024

View reviewed changes

Test that tenants aren't treated as enterprise

4cd58b4

Add normalizeHostname tests

50645d5

williammartin merged commit 45fa8a4 into trunk Mar 4, 2024

williammartin deleted the wm/treat-tenancy-as-non-enterprise branch March 4, 2024 12:21

andyfeller mentioned this pull request Mar 8, 2024

Allow reuse of host checks for extension authors #152

Merged

Navigation Menu

Repository navigation

Treat tenancy as non enterprise - #151

Treat tenancy as non enterprise#151
williammartin merged 3 commits into
trunkfrom
wm/treat-tenancy-as-non-enterprise

williammartin commented Feb 26, 2024

Uh oh!

andyfeller left a comment

Uh oh!

andyfeller Feb 26, 2024

Uh oh!

williammartin Feb 26, 2024 •

edited

Loading

Uh oh!

williammartin commented Feb 26, 2024 •

edited

Loading

Uh oh!

williammartin Feb 28, 2024

Uh oh!

andyfeller commented Feb 28, 2024

Uh oh!

andyfeller left a comment

Uh oh!

williammartin commented Feb 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	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
	}

Conversation

williammartin commented Feb 26, 2024

Description

Uh oh!

andyfeller left a comment

Choose a reason for hiding this comment

Uh oh!

andyfeller Feb 26, 2024

Choose a reason for hiding this comment

Uh oh!

williammartin Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

williammartin commented Feb 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

williammartin Feb 28, 2024

Choose a reason for hiding this comment

Uh oh!

andyfeller commented Feb 28, 2024

Uh oh!

andyfeller left a comment

Choose a reason for hiding this comment

Uh oh!

williammartin commented Feb 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

williammartin Feb 26, 2024 •

edited

Loading

williammartin commented Feb 26, 2024 •

edited

Loading