Use api subdomain for REST and GQL clients when host is tenant #172
Conversation
There was a problem hiding this comment.
I'm torn regarding some of the duplication of this logic within cli/go-gh as well as cli/cli. My knee jerk would be cli/go-gh is the source of truth for this logic and cli/cli leveraging the logic for its needs, which I realize is outside of the scope of the original issue and likely would be deferred.
Beyond that, I wonder how we go about testing these changes in an extension other than gh to ensure they work as expected. Perhaps this is a great opportunity to pair on testing this with gh copilot?
| func isEnterprise(host string) bool { | ||
| return host != github && host != localhost | ||
| return host != github && host != localhost && !isTenancy(host) | ||
| } | ||
|
|
||
| // tenancyHost is the domain name of a tenancy GitHub instance. | ||
| const tenancyHost = "ghe.com" | ||
|
|
||
| func isTenancy(host string) bool { | ||
| return strings.HasSuffix(host, "."+tenancyHost) |
There was a problem hiding this comment.
Are there any concerns about this being seemingly redundant to the logic in auth?
I did a double take when I first saw this because I thought the enterprise check was already taking tenancy into account, when I realized I was thinking about:
Lines 152 to 167 in 25db6b9
My concern is managing 2 sets of similar / overlapping functionality in 2 places and introducing drift.
There was a problem hiding this comment.
See "implementation choices" section in PR description.
I think we should do work here or in a follow up to address this but I'm not going through tearing things up until we know this is works as expected and covers the right use cases.
| // This has been copied over from the cli/cli NormalizeHostname function | ||
| // to ensure compatible behaviour but we don't fully understand when or | ||
| // why it would be useful here. We can't see what harm will come of | ||
| // duplicating the logic. | ||
| if before, found := strings.CutSuffix(hostname, "."+tenancyHost); found { | ||
| idx := strings.LastIndex(before, ".") | ||
| return fmt.Sprintf("%s.%s", before[idx+1:], tenancyHost) | ||
| } |
There was a problem hiding this comment.
Similar to my comment above, it seems we now have 3 copies of this logic across cli/cli and 2 places within cli/go-gh and that makes me a little concerned:
Lines 169 to 186 in 25db6b9
There was a problem hiding this comment.
Same as my response above.
I'm not sure what the difference between Best way would be the same way I suggested in the PR description for |
Description
When targeting the API of tenants we should be using the subdomain rather than the path prefix.
Implementation Choices
Right now, I took the straightest path to implementing this, which included copying functions from the
authpackage. It was an intentional choice not to DRY this out because I want to derisk this work and then review it with confidence that the black box behaviour is correct.How To Test
The easiest way to black box validate these changes is to use the PR in
cli/clithat pins to the commit sha.Alternatively, replace the dependency in
cli/clilike so:Then build and run the CLI with commands that use these clients such as:
In both cases the HTTP requests should demonstrate the API subdomain being used instead of the path prefix.