feat: Add support for listing autolink references #10124

hoffm · 2024-12-22T22:10:19Z

This is the first gh repo autolink subcommand, so this PR adds scaffolding for that command as well. See the linked issue for spec.

jtmcg

Thanks for getting started on this! I've begun reviewing this PR by verifying the AC. I've only encountered one discrepancy

Missing admin rights

Given I don't have the admin role on the repository
And Given I have a local repository cloned from GitHub
When I run gh repo autolink list
Then I see an informative error

error getting autolinks: HTTP 404: Must have admin rights to Repository. (https://api.github.com/repos/{owner}/{repo}/autolinks)

I currently see this error:

HTTP 404: Not Found (https://api.github.com/repos/{owner}/{repo}/autolinks)

It appears that the api response doesn't specify that you don't have the sufficient permissions and just responds with the 404. Since you've piped the error directly into the response here, it makes sense that this is the limit of the error message.

The AC asks for a more helpful error message, so we should probably wrap this error in a custom error response. I've made note of where I think this needs to happen in the comments review comments.

Otherwise, all the other AC seems to be working as outlined.

I have added a few other comments for your consideration. There's one in there about the testing pattern this has copied that is one of my personal pain-points in gh development... I'm not gonna block this PR on it, but I do plan to open a branch against this one to at least demonstrate how I would do this differently so that we can get away from this over-mocking pattern.

I'll keep you posted on the status of that.

Overall, excellent work, and thank you for getting after this!

pkg/cmd/repo/autolink/autolink.go

jtmcg · 2024-12-23T20:54:46Z

pkg/cmd/repo/autolink/http.go

+	if resp.StatusCode > 299 {
+		return nil, api.HandleHTTPError(resp)
+	}


The AC mentions helpful error messaging for 404's

error getting autolinks: HTTP 404: Must have admin rights to Repository. (https://api.github.com/repos/{owner}/{repo}/autolinks)

I think that would require an extra conditional, here:

Suggested change

if resp.StatusCode > 299 {

return nil, api.HandleHTTPError(resp)

}

if resp.StatusCode == 404 {

return nil, fmt.Errorf("error getting autolinks: HTTP 404: Must have admin rights to Repository. (https://api.github.com/repos/%s)", path)

} else if resp.StatusCode > 299 {

return nil, api.HandleHTTPError(resp)

}

Note, this suppresses the actual error returned by the api for 404's. Maybe that's intentional, but I'd look to @andyfeller to chime in, here.

No need to block merging this on his response, though. We can always open a follow-up PR to change the error handling if the AC needs revisiting 🙂

This messaging was added via hoffm#1. I'll leave the comment unresolved for @andyfeller's benefit.

Interesting 🤔 when I was originally authoring the AC, I was basing this on the response provided back by gh api making the necessary calls.

@jtmcg makes a good point that it needs to be easy for users to understand if they don't have the right permission and what they should do to correct it. In this case, it isn't something trivial like gh auth refresh -s <SCOPE> but the role assigned to you.

Let me catch up on the rest of these changes as I trust y'all worked something up while I was OoO.

pkg/cmd/repo/autolink/list_test.go

pkg/cmd/repo/autolink/list.go

williammartin

Overall this is looking really good, thank you for your work.

pkg/cmd/repo/autolink/list/http.go

pkg/cmd/repo/autolink/list/list_test.go

pkg/cmd/repo/autolink/list/http.go

williammartin · 2024-12-24T12:21:45Z

pkg/cmd/repo/autolink/list/http.go

+
+	if resp.StatusCode == 404 {
+		return nil, fmt.Errorf("error getting autolinks: HTTP 404: Must have admin rights to Repository. (https://api.github.com/%s)", path)
+	} else if resp.StatusCode > 299 {


Food For Thought

Not for now but it really feels like this pattern should be pulled into a function. It's used everywhere and I think giving it a name would really help with understanding the intent.

pkg/cmd/repo/autolink/list/list_test.go

…ting This defines an AutolinkClient interface with a Get method used for fetching the autolinks lists from the api. Then, the http client for autolinks implements this interface with the AutolinkGetter struct. This allows for dependency injection of the AutolinkGetter struct into the listOptions, enabling mocking of the AutolinkGetter for testing. The result of this is simpler tests that are easier to maintain, because the interface for the table tests now allow for defining autolink structs as the response instead of large mocked api calls. This also allows for bespoke testing of the http file, which I'll follow up with in the next commit.

This adds the missing mocked http tests to the http_test.go file. These tests were previously bundled with the tests in list_test.go, creating a testing pattern that was difficult to understand and maintain. The refactor in the previous commit replaced these tests with the AutolinkClient interface, allowing for the httpmocks to be isolated to the AutolinkGetter that implements that interface.

Co-authored-by: Tyler McGoffin <jtmcg@github.com>

jtmcg

This is looking really good! I've got only the one small comment but would otherwise give this my ✅

pkg/cmd/repo/autolink/list/list_test.go

jtmcg

🔥 Woot! Thanks for all the work to get this one over the line, @hoffm!

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [cli/cli](https://github.com/cli/cli) | minor | `v2.64.0` -> `v2.65.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.65.0`](https://github.com/cli/cli/releases/tag/v2.65.0): GitHub CLI 2.65.0 [Compare Source](cli/cli@v2.64.0...v2.65.0) #### What's Changed - Document the base repo resolution functions by [@williammartin](https://github.com/williammartin) in cli/cli#10110 - Update releasing.md by [@andyfeller](https://github.com/andyfeller) in cli/cli#10116 - Document how to set gh-merge-base by [@heaths](https://github.com/heaths) in cli/cli#10112 - Upgrade golang.org/x/net to v0.33.0 by [@jtmcg](https://github.com/jtmcg) in cli/cli#10135 - add pending status for workflow runs by [@dziamidchyk](https://github.com/dziamidchyk) in cli/cli#10143 - Remove release discussion posts and clean up related block in deployment yml by [@shauryatiwari1](https://github.com/shauryatiwari1) in cli/cli#10145 - docs(repo): make explicit which branch is used when creating a repo by [@nobe4](https://github.com/nobe4) in cli/cli#10163 - feat: Add support for listing autolink references by [@hoffm](https://github.com/hoffm) in cli/cli#10124 - Add mention of classic token in gh auth login docs by [@jtmcg](https://github.com/jtmcg) in cli/cli#10164 - Feat: Allow setting security_and_analysis settings in gh repo edit by [@ChandranshuRao14](https://github.com/ChandranshuRao14) in cli/cli#10139 - Upgrade generated workflows by [@jsoref](https://github.com/jsoref) in cli/cli#10181 - Myriad fixes to provide clarity on determining tracking ref in MR create by [@williammartin](https://github.com/williammartin) in cli/cli#10187 - Handle missing upstream configs for `gh pr create` by [@cmbrose](https://github.com/cmbrose) in cli/cli#10177 - fix(repo fork): add non-TTY output when fork is newly created by [@aryanbhosale](https://github.com/aryanbhosale) in cli/cli#10158 - Bump cli/go-gh for indirect security vulnerability by [@andyfeller](https://github.com/andyfeller) in cli/cli#10190 #### New Contributors - [@dziamidchyk](https://github.com/dziamidchyk) made their first contribution in cli/cli#10143 - [@shauryatiwari1](https://github.com/shauryatiwari1) made their first contribution in cli/cli#10145 - [@hoffm](https://github.com/hoffm) made their first contribution in cli/cli#10124 - [@ChandranshuRao14](https://github.com/ChandranshuRao14) made their first contribution in cli/cli#10139 **Full Changelog**: cli/cli@v2.64.0...v2.65.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).

hoffm requested a review from a team as a code owner December 22, 2024 22:10

hoffm requested a review from jtmcg December 22, 2024 22:10

cliAutomation added the external pull request originating outside of the CLI core team label Dec 22, 2024

hoffm force-pushed the autolink-references branch 2 times, most recently from e5c63a1 to c59e236 Compare December 23, 2024 13:35

jtmcg reviewed Dec 23, 2024

View reviewed changes

jtmcg mentioned this pull request Dec 23, 2024

Refactor http handler to interface pattern for simpler testing in autolinks hoffm/github-cli#1

Merged

hoffm requested a review from jtmcg December 24, 2024 02:13

williammartin reviewed Dec 24, 2024

View reviewed changes

pkg/cmd/repo/autolink/list/list_test.go Outdated Show resolved Hide resolved

williammartin reviewed Dec 24, 2024

View reviewed changes

pkg/cmd/repo/autolink/list/list_test.go Outdated Show resolved Hide resolved

hoffm and others added 7 commits December 27, 2024 21:36

Introduce repo autolinks list commands

23c16c9

Apply PR comment changes

5fb9852

Refactor out early return in test code

869d251

Co-authored-by: Tyler McGoffin <jtmcg@github.com>

Whitespace

ea04d2d

Refactor autolink subcommands into their own packages

e98ff2e

hoffm force-pushed the autolink-references branch from 955bd6b to e98ff2e Compare December 28, 2024 02:36

hoffm added 6 commits December 27, 2024 21:40

PR nits

67266e9

Break out autolink list json fields test

cc24289

Remove NewAutolinkClient

dc6320f

Use 'list' instead of 'get' for autolink list type and method

63488a1

Decode instead of unmarshal

20f0865

Better error testing for autolink TestListRun

da826db

hoffm requested a review from williammartin December 28, 2024 03:59

Complete get -> list renaming

fa254ba

hoffm mentioned this pull request Dec 30, 2024

Add support for gh repo autolink create #10119

Closed

jtmcg reviewed Jan 2, 2025

View reviewed changes

pkg/cmd/repo/autolink/list/list_test.go Outdated Show resolved Hide resolved

hoffm added 2 commits January 2, 2025 13:11

Merge branch 'cli:trunk' into autolink-references

8f00cd4

Separate type decrarations

a5cf375

hoffm requested a review from jtmcg January 2, 2025 18:15

jtmcg approved these changes Jan 2, 2025

View reviewed changes

jtmcg enabled auto-merge January 2, 2025 22:20

jtmcg merged commit 2306623 into cli:trunk Jan 2, 2025

jtmcg mentioned this pull request Jan 15, 2025

feat: Add support for creating autolink references #10180

Merged

feat: Add support for listing autolink references #10124

feat: Add support for listing autolink references #10124

Uh oh!

Conversation

hoffm commented Dec 22, 2024

Uh oh!

jtmcg left a comment

Choose a reason for hiding this comment

Missing admin rights

Uh oh!

Uh oh!

jtmcg Dec 23, 2024

Choose a reason for hiding this comment

Uh oh!

hoffm Dec 24, 2024

Choose a reason for hiding this comment

Uh oh!

andyfeller Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

williammartin left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

williammartin Dec 24, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jtmcg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jtmcg left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants