Update IsEnterprise and IsTenancy for orthogonality using go-gh by jtmcg · Pull Request #9755 · cli/cli

jtmcg · 2024-10-14T18:41:13Z

Closes github/cli#577

Changes

Add test coverage to places where IsEnterprise incorrectly covers Tenancy
Upgrade go-gh to v2.11.0
Use go-gh/auth package for IsEnterprise, IsTenancy, and NormalizeHostname
auth: Removed redundant ghauth.IsTenancy(host) check

This adds test coverage to ensure orthogonality between IsEnterprise and IsTenancy functionality, then imports the functions from go-gh to fix the tests. There's some additional cleanup beyond this as well. See the commit messages for more details.

Testing

Pull down branch gh pr checkout 9755
Run all tests
Login and logout with a tenancy stamp to verify the flow is working: gh auth login -h <tenancy-instance>
Login and logout of a server stamp to verify the flow is working: gh auth login -h <server-instance>

andyfeller

Feels good to get rid of this logic and standardized on a single reusable set of functions! Handful of nits but solid overall.

internal/featuredetection/feature_detection_test.go

internal/ghinstance/host_test.go

pkg/cmd/gist/clone/clone_test.go

…ancy IsEnterprise currently returns `true` when `IsTenancy` returns true. We prefer that this behavior is orthogonal. This commit adds failing tests to the instances where IsEnterprise should not behave the same as IsTenancy. These test cases are expected to pass with the inclusion of the coming improvements to go-gh to handle the orthogonality of IsEnterprise and IsTenancy.

…name

andyfeller

lgtm! so glad this finally happened!

williammartin

I think that these branches can probably be removed now:

cli/internal/ghinstance/host.go

Lines 50 to 56 in e1a39d8

    
           // Once we change Tenancy to no longer be treated as Enterprise, this 
        
           // conditional can be removed as the flow will fall through to the bottom. 
        
           // However, we can't do that until we've investigated all places in which 
        
           // Tenancy is currently treated as Enterprise. 
        
           if ghauth.IsTenancy(hostname) { 
        
           	return fmt.Sprintf("https://api.%s/graphql", hostname) 
        
           }

cli/internal/ghinstance/host.go

Lines 70 to 76 in e1a39d8

    
           // Once we change Tenancy to no longer be treated as Enterprise, this 
        
           // conditional can be removed as the flow will fall through to the bottom. 
        
           // However, we can't do that until we've investigated all places in which 
        
           // Tenancy is currently treated as Enterprise. 
        
           if ghauth.IsTenancy(hostname) { 
        
           	return fmt.Sprintf("https://api.%s/", hostname) 
        
           }

Left a couple of comments for you to consider but otherwise LGTM thanks.

pkg/cmd/pr/status/http.go

pkg/cmd/gist/clone/clone.go

jtmcg · 2024-10-15T19:36:08Z

I think that these branches can probably be removed now:

Thanks for the callout, @williammartin. I've removed them 🙌

phillmv

Seems straightforward for the parts @cli/package-security owns.

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [cli/cli](https://github.com/cli/cli) | minor | `v2.58.0` -> `v2.59.0` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>cli/cli (cli/cli)</summary> ### [`v2.59.0`](https://github.com/cli/cli/releases/tag/v2.59.0): GitHub CLI 2.59.0 [Compare Source](cli/cli@v2.58.0...v2.59.0) #### What's Changed - Allow community submitted design work by [@BagToad](https://github.com/BagToad) in cli/cli#9683 - Improve `SECURITY.md` with expectations for privately reported vulnerabilities by [@BagToad](https://github.com/BagToad) in cli/cli#9687 - Emit a log message when extension installation falls back to a `darwin-amd64` binary on an Apple Silicon macOS device by [@timrogers](https://github.com/timrogers) in cli/cli#9650 - Print the login URL even when opening a browser by [@ulfjack](https://github.com/ulfjack) in cli/cli#7091 - configurable maxwidth for markdown WithWrap() by [@smemsh](https://github.com/smemsh) in cli/cli#9626 - Handle errors when parsing hostname in auth flow by [@BagToad](https://github.com/BagToad) in cli/cli#9729 - Add `repo license list/view` and `repo gitignore list/view` by [@BagToad](https://github.com/BagToad) in cli/cli#9721 - Introduce testscript acceptance tests generally, and for the MR command specifically by [@williammartin](https://github.com/williammartin) in cli/cli#9745 - Support `GH_ACCEPTANCE_SCRIPT` env var to target a single script by [@williammartin](https://github.com/williammartin) in cli/cli#9756 - Ensure Acceptance defer failures are debuggable by [@williammartin](https://github.com/williammartin) in cli/cli#9754 - Add acceptance task to makefile by [@williammartin](https://github.com/williammartin) in cli/cli#9748 - Add Acceptance tests for `issue` command by [@williammartin](https://github.com/williammartin) in cli/cli#9757 - Update IsEnterprise and IsTenancy for orthogonality using go-gh by [@jtmcg](https://github.com/jtmcg) in cli/cli#9755 - Supporting filtering on `gist list` by [@heaths](https://github.com/heaths) in cli/cli#9728 #### New Contributors - [@ulfjack](https://github.com/ulfjack) made their first contribution in cli/cli#7091 - [@smemsh](https://github.com/smemsh) made their first contribution in cli/cli#9626 **Full Changelog**: cli/cli@v2.58.0...v2.59.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

jtmcg force-pushed the jtmcg/577 branch 2 times, most recently from feea80b to 66d5dd3 Compare October 14, 2024 21:11

jtmcg marked this pull request as ready for review October 14, 2024 21:23

jtmcg requested review from a team as code owners October 14, 2024 21:23

jtmcg requested a review from williammartin October 14, 2024 21:23

cliAutomation added the external pull request originating outside of the CLI core team label Oct 14, 2024

jtmcg removed the external pull request originating outside of the CLI core team label Oct 14, 2024

andyfeller suggested changes Oct 15, 2024

View reviewed changes

jtmcg added 4 commits October 15, 2024 11:56

Upgrade go-gh version to 2.11.0

44fdb33

Use go-gh/auth package for IsEnterprise, IsTenancy, and NormalizeHost…

81591a0

…name

auth: Removed redundant ghauth.IsTenancy(host) check

8109594

jtmcg force-pushed the jtmcg/577 branch from 66d5dd3 to 8109594 Compare October 15, 2024 18:56

andyfeller approved these changes Oct 15, 2024

View reviewed changes

Add ghe.com to tests describing ghec data residency

e1a39d8

williammartin reviewed Oct 15, 2024

View reviewed changes

pkg/cmd/pr/status/http.go Show resolved Hide resolved

pkg/cmd/gist/clone/clone.go Outdated Show resolved Hide resolved

Remove unnecessary code branches

0bd4e03

jtmcg added 2 commits October 15, 2024 12:41

Remove IsTenancy and relevant tests from gists as they are unsupported

ddf90c7

Merge branch 'trunk' into jtmcg/577

2d0b742

phillmv approved these changes Oct 15, 2024

View reviewed changes

jtmcg merged commit b98c840 into trunk Oct 15, 2024

jtmcg deleted the jtmcg/577 branch October 15, 2024 21:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update IsEnterprise and IsTenancy for orthogonality using go-gh#9755

Update IsEnterprise and IsTenancy for orthogonality using go-gh#9755
jtmcg merged 8 commits intotrunkfrom
jtmcg/577

jtmcg commented Oct 14, 2024 •

edited

Loading

Uh oh!

andyfeller left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andyfeller left a comment

Uh oh!

williammartin left a comment

Uh oh!

Uh oh!

Uh oh!

jtmcg commented Oct 15, 2024

Uh oh!

phillmv left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	// Once we change Tenancy to no longer be treated as Enterprise, this
	// conditional can be removed as the flow will fall through to the bottom.
	// However, we can't do that until we've investigated all places in which
	// Tenancy is currently treated as Enterprise.
	if ghauth.IsTenancy(hostname) {
	return fmt.Sprintf("https://api.%s/graphql", hostname)
	}

Conversation

jtmcg commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Testing

Uh oh!

andyfeller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andyfeller left a comment

Choose a reason for hiding this comment

Uh oh!

williammartin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jtmcg commented Oct 15, 2024

Uh oh!

phillmv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jtmcg commented Oct 14, 2024 •

edited

Loading