From 78836097c31210faae40dff535c48817e6ff6dc4 Mon Sep 17 00:00:00 2001 From: Heath Stewart Date: Fri, 20 Dec 2024 10:49:38 -0800 Subject: [PATCH 01/50] Document how to set gh-merge-base Follow-up to PR #9712 --- pkg/cmd/pr/create/create.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index f3bd12870..3bfa768b7 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -121,7 +121,8 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co The base branch for the created PR can be specified using the %[1]s--base%[1]s flag. If not provided, the value of %[1]sgh-merge-base%[1]s git branch config will be used. If not configured, the repository's - default branch will be used. + default branch will be used. Run %[1]sgit config branch.{current}.gh-merge-base {base}%[1]s to configure + the current branch to use the specified merge base. Link an issue to the pull request by referencing the issue in the body of the pull request. If the body text mentions %[1]sFixes #123%[1]s or %[1]sCloses #123%[1]s, the referenced issue From 0e191ef0f2f9e857d5f1ff6c4b626aa35a19ad01 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Fri, 20 Dec 2024 15:34:26 -0500 Subject: [PATCH 02/50] Update releasing.md --- docs/releasing.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/docs/releasing.md b/docs/releasing.md index d9b01ea41..4b977efdd 100644 --- a/docs/releasing.md +++ b/docs/releasing.md @@ -17,7 +17,7 @@ What this does is: - A new git tag `vX.Y.Z` is created in the remote repository; - The changelog is [generated from the list of merged pull requests](https://docs.github.com/en/repositories/releasing-projects-on-github/automatically-generated-release-notes); - Updates cli.github.com with the contents of the new release; -- Updates our Homebrew formula in the homebrew-core repo. +- Updates the [`gh` Homebrew formula](https://github.com/williammartin/homebrew-core/blob/master/Formula/g/gh.rb) in the [`homebrew/homebrew-core` repo](https://github.com/search?q=repo%3AHomebrew%2Fhomebrew-core+%22gh%22+in%3Atitle&type=pullrequests). To test out the build system while avoiding creating an actual release: ```sh @@ -38,3 +38,12 @@ A local release can be created for testing without creating anything official on 1. Make sure GoReleaser is installed: `brew install goreleaser` 2. `script/release --local` 3. Find the built products under `dist/`. + +## Cleaning up a bad release + +Occasionally, it might be necessary to clean up a bad release and re-release. + +1. Delete the release and associated tag +2. Re-release and monitor the workflow run logs +3. Open pull request updating [`gh` Homebrew formula](https://github.com/williammartin/homebrew-core/blob/master/Formula/g/gh.rb) with new SHA versions, linking the previous PR +4. Verify resulting Debian and RPM packages, Homebrew formula From 7a954cde829af17938db77568573f07fbc92a867 Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 20 Dec 2024 15:20:49 +0100 Subject: [PATCH 03/50] Document BaseRepoFunc --- pkg/cmd/factory/default.go | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 7abf3ca5f..1551a8758 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -44,6 +44,31 @@ func New(appVersion string) *cmdutil.Factory { return f } +// BaseRepoFunc requests a list of Remotes, and selects the first one. +// Although Remotes is injected via the factory so it looks like the function might +// be configurable, in practice, it's calling readRemotes, and the injection is indirection. +// +// readRemotes makes use of the remoteResolver, which is responsible for requesting the list +// of remotes for the current working directory from git. It then does some filtering to +// only retain remotes for hosts that we have authenticated against; keep in mind this may +// be the single value of GH_HOST. +// +// That list of remotes is sorted by their remote name, in the following order: +// 1. upstream +// 2. github +// 3. origin +// 4. other remotes, no ordering guaratanteed because the sort function is not stable +// +// Given that list, this function chooses the first one. +// +// Here's a common example of when this might matter: when we clone a fork, by default we add +// the parent as a remote named upstream. So the remotes may look like this: +// upstream https://github.com/cli/cli.git (fetch) +// upstream https://github.com/cli/cli.git (push) +// origin https://github.com/cli/cli-fork.git (fetch) +// origin https://github.com/cli/cli-fork.git (push) +// +// With this resolution function, the upstream will always be chosen (assuming we have authenticated with github.com). func BaseRepoFunc(f *cmdutil.Factory) func() (ghrepo.Interface, error) { return func() (ghrepo.Interface, error) { remotes, err := f.Remotes() From b9c597458b315c1237a16bd883f5f064d94be6a3 Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 20 Dec 2024 15:51:36 +0100 Subject: [PATCH 04/50] Document SmartBaseRepoFunc --- pkg/cmd/factory/default.go | 72 ++++++++++++++++++++++++++++++++++++-- 1 file changed, 70 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 1551a8758..0e8bd7856 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -79,6 +79,74 @@ func BaseRepoFunc(f *cmdutil.Factory) func() (ghrepo.Interface, error) { } } +// SmartBaseRepoFunc provides additional behaviour over BaseRepoFunc. Read the BaseRepoFunc +// documentation for more information on how remotes are fetched and ordered. +// +// Unlike BaseRepoFunc, instead of selecting the first remote in the list, this function will +// use the API to resolve repository networks, and attempt to use the `resolved` git remote config value +// as part of determining the base repository. +// +// Although the behaviour commented below really belongs to the `BaseRepo` function on `ResolvedRemotes`, +// in practice the most important place to understand the general behaviour is here, so that's where +// I'm going to write it. +// +// Firstly, the remotes are inspected to see whether any are already resolved. Resolution means the git +// config value of the `resolved` key was `base` (meaning this remote is the base repository), or a specific +// repository e.g. `cli/cli` (meaning that specific repo is the base repo, regardless of whether a remote +// exists for it). These values are set by default on clone of a fork, or by running `repo set-default`. If +// either are set, that repository is returned. +// +// If we the current invocation is unable to prompt, then the first remote is returned. I believe this behaviour +// exists for backwards compatibility before the later steps were introduced, however, this is frequently a source +// of differing behaviour between interactive and non-interactive invocations: +// +// ➜ git remote -v +// origin https://github.com/williammartin/test-repo.git (fetch) +// origin https://github.com/williammartin/test-repo.git (push) +// upstream https://github.com/williammartin-test-org/test-repo.git (fetch) +// upstream https://github.com/williammartin-test-org/test-repo.git (push) +// +// ➜ gh pr list +// X No default remote repository has been set for this directory. +// +// please run `gh repo set-default` to select a default remote repository. +// ➜ gh pr list | cat +// 3 test williammartin-test-org:remote-push-default-feature OPEN 2024-12-13T10:28:40Z +// +// Furthermore, when repositories have been renamed on the server and not on the local git remote, this causes +// even more confusion because the API requests can be different, and FURTHERMORE this can be an issue for +// services that don't handle renames correctly, like the ElasticSearch indexing. +// +// Assuming we have an interactive invocation, then the next step is to resolve a network of respositories. This +// involves creating a dynamic GQL query requesting information about each repository (up to a limit of 5). +// Each returned repo is added to a list, along with its parent, if present in the query response. +// The repositories in the query retain the same ordering as previously outlined. Interestingly, the request is sent +// to the hostname of the first repo, so if you happen to have remotes on different GitHub hosts, then they won't +// resolve correctly. I'm not sure this has ever caused an issue, but does seem like a potential source of bugs. +// In practice, since the remotes are ordered with upstream, github, origin before others, it's almost always going +// to be the case that the correct host is chosen. +// +// Because fetching the network includes the parent repo, even if it is not a remote, this requires the user to +// disambiguate, which can be surprising, though I'm not sure I've heard anyone complain: +// +// ➜ git remote -v +// origin https://github.com/williammartin/test-repo.git (fetch) +// origin https://github.com/williammartin/test-repo.git (push) +// +// ➜ gh pr list +// X No default remote repository has been set for this directory. +// +// please run `gh repo set-default` to select a default remote repository. +// +// If no repos are returned from the API then we return the first remote from the original list. I'm not sure +// why we do this rather than erroring, because it seems like almost every future step is going to fail when hitting +// the API. Potentially it helps if there is an API blip? It was added without comment in: +// https://github.com/cli/cli/pull/1706/files#diff-65730f0373fb91dd749940cf09daeaf884e5643d665a6c3eb09d54785a6d475eR113 +// +// If one repo is returned from the API, then that one is returned as the base repo. +// +// If more than one repo is returned from the API, we indicate to the user that they need to run `repo set-default`, +// and return an error with no base repo. func SmartBaseRepoFunc(f *cmdutil.Factory) func() (ghrepo.Interface, error) { return func() (ghrepo.Interface, error) { httpClient, err := f.HttpClient() @@ -92,11 +160,11 @@ func SmartBaseRepoFunc(f *cmdutil.Factory) func() (ghrepo.Interface, error) { if err != nil { return nil, err } - repoContext, err := ghContext.ResolveRemotesToRepos(remotes, apiClient, "") + resolvedRepos, err := ghContext.ResolveRemotesToRepos(remotes, apiClient, "") if err != nil { return nil, err } - baseRepo, err := repoContext.BaseRepo(f.IOStreams) + baseRepo, err := resolvedRepos.BaseRepo(f.IOStreams) if err != nil { return nil, err } From 939e183cd62572616c80982199a04652f4203c2d Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Mon, 23 Dec 2024 11:36:09 -0800 Subject: [PATCH 05/50] Upgrade golang.org/x/net to v0.33.0 --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 5191e3c78..98ee0280f 100644 --- a/go.mod +++ b/go.mod @@ -159,7 +159,7 @@ require ( go.uber.org/zap v1.27.0 // indirect golang.org/x/exp v0.0.0-20240112132812-db7319d0e0e3 // indirect golang.org/x/mod v0.21.0 // indirect - golang.org/x/net v0.31.0 // indirect + golang.org/x/net v0.33.0 // indirect golang.org/x/sys v0.28.0 // indirect golang.org/x/tools v0.26.0 // indirect google.golang.org/genproto/googleapis/api v0.0.0-20240520151616-dc85e6b867a5 // indirect diff --git a/go.sum b/go.sum index 0777687a8..49e104d07 100644 --- a/go.sum +++ b/go.sum @@ -498,8 +498,8 @@ golang.org/x/mod v0.21.0/go.mod h1:6SkKJ3Xj0I0BrPOZoBy3bdMptDDU9oJrpohJ3eWZ1fY= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= golang.org/x/net v0.0.0-20220722155237-a158d28d115b/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c= -golang.org/x/net v0.31.0 h1:68CPQngjLL0r2AlUKiSxtQFKvzRVbnzLwMUn5SzcLHo= -golang.org/x/net v0.31.0/go.mod h1:P4fl1q7dY2hnZFxEk4pPSkDHF+QqjitcnDjUQyMM+pM= +golang.org/x/net v0.33.0 h1:74SYHlV8BIgHIFC/LrYkOGIwL19eTYXQ5wc6TBuO36I= +golang.org/x/net v0.33.0/go.mod h1:HXLR5J+9DxmrqMwG9qjGCxZ+zKXxBru04zlTvWlWuN4= golang.org/x/oauth2 v0.22.0 h1:BzDx2FehcG7jJwgWLELCdmLuxk2i+x9UDpSiss2u0ZA= golang.org/x/oauth2 v0.22.0/go.mod h1:XYTD2NtWslqkgxebSiOHnXEap4TF09sJSc7H1sXbhtI= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= From a542011cdce8fb579089a0e6a8284526a19d21d3 Mon Sep 17 00:00:00 2001 From: ChandranshuRao14 Date: Tue, 24 Dec 2024 12:59:42 -0600 Subject: [PATCH 06/50] Feat: Allow setting security_and_analysis settings in gh repo edit --- pkg/cmd/repo/edit/edit.go | 92 ++++++++++++++++++++++++++++------ pkg/cmd/repo/edit/edit_test.go | 30 +++++++++++ 2 files changed, 106 insertions(+), 16 deletions(-) diff --git a/pkg/cmd/repo/edit/edit.go b/pkg/cmd/repo/edit/edit.go index 29a149cf9..80e460186 100644 --- a/pkg/cmd/repo/edit/edit.go +++ b/pkg/cmd/repo/edit/edit.go @@ -66,22 +66,37 @@ type EditOptions struct { } type EditRepositoryInput struct { - AllowForking *bool `json:"allow_forking,omitempty"` - AllowUpdateBranch *bool `json:"allow_update_branch,omitempty"` - DefaultBranch *string `json:"default_branch,omitempty"` - DeleteBranchOnMerge *bool `json:"delete_branch_on_merge,omitempty"` - Description *string `json:"description,omitempty"` - EnableAutoMerge *bool `json:"allow_auto_merge,omitempty"` - EnableIssues *bool `json:"has_issues,omitempty"` - EnableMergeCommit *bool `json:"allow_merge_commit,omitempty"` - EnableProjects *bool `json:"has_projects,omitempty"` - EnableDiscussions *bool `json:"has_discussions,omitempty"` - EnableRebaseMerge *bool `json:"allow_rebase_merge,omitempty"` - EnableSquashMerge *bool `json:"allow_squash_merge,omitempty"` - EnableWiki *bool `json:"has_wiki,omitempty"` - Homepage *string `json:"homepage,omitempty"` - IsTemplate *bool `json:"is_template,omitempty"` - Visibility *string `json:"visibility,omitempty"` + enableAdvancedSecurity *bool + enableSecretScanning *bool + enableSecretScanningPushProtection *bool + + AllowForking *bool `json:"allow_forking,omitempty"` + AllowUpdateBranch *bool `json:"allow_update_branch,omitempty"` + DefaultBranch *string `json:"default_branch,omitempty"` + DeleteBranchOnMerge *bool `json:"delete_branch_on_merge,omitempty"` + Description *string `json:"description,omitempty"` + EnableAutoMerge *bool `json:"allow_auto_merge,omitempty"` + EnableIssues *bool `json:"has_issues,omitempty"` + EnableMergeCommit *bool `json:"allow_merge_commit,omitempty"` + EnableProjects *bool `json:"has_projects,omitempty"` + EnableDiscussions *bool `json:"has_discussions,omitempty"` + EnableRebaseMerge *bool `json:"allow_rebase_merge,omitempty"` + EnableSquashMerge *bool `json:"allow_squash_merge,omitempty"` + EnableWiki *bool `json:"has_wiki,omitempty"` + Homepage *string `json:"homepage,omitempty"` + IsTemplate *bool `json:"is_template,omitempty"` + SecurityAndAnalysis *SecurityAndAnalysisInput `json:"security_and_analysis,omitempty"` + Visibility *string `json:"visibility,omitempty"` +} + +type SecurityAndAnalysisInput struct { + EnableAdvancedSecurity *SecurityAndAnalysisStatus `json:"advanced_security,omitempty"` + EnableSecretScanning *SecurityAndAnalysisStatus `json:"secret_scanning,omitempty"` + EnableSecretScanningPushProtection *SecurityAndAnalysisStatus `json:"secret_scanning_push_protection,omitempty"` +} + +type SecurityAndAnalysisStatus struct { + Status *string `json:"status,omitempty"` } func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobra.Command { @@ -177,6 +192,9 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobr cmdutil.NilBoolFlag(cmd, &opts.Edits.EnableSquashMerge, "enable-squash-merge", "", "Enable merging pull requests via squashed commit") cmdutil.NilBoolFlag(cmd, &opts.Edits.EnableRebaseMerge, "enable-rebase-merge", "", "Enable merging pull requests via rebase") cmdutil.NilBoolFlag(cmd, &opts.Edits.EnableAutoMerge, "enable-auto-merge", "", "Enable auto-merge functionality") + cmdutil.NilBoolFlag(cmd, &opts.Edits.enableAdvancedSecurity, "enable-advanced-security", "", "Enable advanced security in the repository") + cmdutil.NilBoolFlag(cmd, &opts.Edits.enableSecretScanning, "enable-secret-scanning", "", "Enable secret scanning in the repository") + cmdutil.NilBoolFlag(cmd, &opts.Edits.enableSecretScanningPushProtection, "enable-secret-scanning-push-protection", "", "Enable secret scanning push protection in the repository. Secret scanning must be enabled first") cmdutil.NilBoolFlag(cmd, &opts.Edits.DeleteBranchOnMerge, "delete-branch-on-merge", "", "Delete head branch when pull requests are merged") cmdutil.NilBoolFlag(cmd, &opts.Edits.AllowForking, "allow-forking", "", "Allow forking of an organization repository") cmdutil.NilBoolFlag(cmd, &opts.Edits.AllowUpdateBranch, "allow-update-branch", "", "Allow a pull request head branch that is behind its base branch to be updated") @@ -240,6 +258,34 @@ func editRun(ctx context.Context, opts *EditOptions) error { } } + if hasSecurityEdits(opts.Edits) { + apiClient := api.NewClientFromHTTP(opts.HTTPClient) + repo, err := api.FetchRepository(apiClient, opts.Repository, []string{"viewerCanAdminister"}) + if err != nil { + return err + } + if !repo.ViewerCanAdminister { + return fmt.Errorf("you do not have sufficient permissions to edit repository security and analysis features") + } + + opts.Edits.SecurityAndAnalysis = &SecurityAndAnalysisInput{} + if opts.Edits.enableAdvancedSecurity != nil { + opts.Edits.SecurityAndAnalysis.EnableAdvancedSecurity = &SecurityAndAnalysisStatus{ + Status: boolToStatus(*opts.Edits.enableAdvancedSecurity), + } + } + if opts.Edits.enableSecretScanning != nil { + opts.Edits.SecurityAndAnalysis.EnableSecretScanning = &SecurityAndAnalysisStatus{ + Status: boolToStatus(*opts.Edits.enableSecretScanning), + } + } + if opts.Edits.enableSecretScanningPushProtection != nil { + opts.Edits.SecurityAndAnalysis.EnableSecretScanningPushProtection = &SecurityAndAnalysisStatus{ + Status: boolToStatus(*opts.Edits.enableSecretScanningPushProtection), + } + } + } + apiPath := fmt.Sprintf("repos/%s/%s", repo.RepoOwner(), repo.RepoName()) body := &bytes.Buffer{} @@ -560,3 +606,17 @@ func isIncluded(value string, opts []string) bool { } return false } + +func boolToStatus(status bool) *string { + var result string + if status { + result = "enabled" + } else { + result = "disabled" + } + return &result +} + +func hasSecurityEdits(edits EditRepositoryInput) bool { + return edits.enableAdvancedSecurity != nil || edits.enableSecretScanning != nil || edits.enableSecretScanningPushProtection != nil +} diff --git a/pkg/cmd/repo/edit/edit_test.go b/pkg/cmd/repo/edit/edit_test.go index 217c1dce4..93d44788d 100644 --- a/pkg/cmd/repo/edit/edit_test.go +++ b/pkg/cmd/repo/edit/edit_test.go @@ -201,6 +201,36 @@ func Test_editRun(t *testing.T) { })) }, }, + { + name: "enable/disable security and analysis settings", + opts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + SecurityAndAnalysis: &SecurityAndAnalysisInput{ + EnableAdvancedSecurity: &SecurityAndAnalysisStatus{ + Status: sp("enabled"), + }, + EnableSecretScanning: &SecurityAndAnalysisStatus{ + Status: sp("enabled"), + }, + EnableSecretScanningPushProtection: &SecurityAndAnalysisStatus{ + Status: sp("disabled"), + }, + }, + }, + }, + httpStubs: func(t *testing.T, r *httpmock.Registry) { + r.Register( + httpmock.REST("PATCH", "repos/OWNER/REPO"), + httpmock.RESTPayload(200, `{}`, func(payload map[string]interface{}) { + assert.Equal(t, 1, len(payload)) + securityAndAnalysis := payload["security_and_analysis"].(map[string]interface{}) + assert.Equal(t, "enabled", securityAndAnalysis["advanced_security"].(map[string]interface{})["status"]) + assert.Equal(t, "enabled", securityAndAnalysis["secret_scanning"].(map[string]interface{})["status"]) + assert.Equal(t, "disabled", securityAndAnalysis["secret_scanning_push_protection"].(map[string]interface{})["status"]) + })) + }, + }, } for _, tt := range tests { From f9767d0a9aa5cfae4454a43b6ceae9b23c20faf2 Mon Sep 17 00:00:00 2001 From: Arseni Dziamidchyk Date: Thu, 26 Dec 2024 19:10:06 +0300 Subject: [PATCH 07/50] add pending status for workflow runs --- pkg/cmd/run/shared/shared.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/cmd/run/shared/shared.go b/pkg/cmd/run/shared/shared.go index 51eb547bc..a583f3a0b 100644 --- a/pkg/cmd/run/shared/shared.go +++ b/pkg/cmd/run/shared/shared.go @@ -27,6 +27,7 @@ const ( InProgress Status = "in_progress" Requested Status = "requested" Waiting Status = "waiting" + Pending Status = "pending" // Run conclusions ActionRequired Conclusion = "action_required" @@ -53,6 +54,7 @@ var AllStatuses = []string{ "in_progress", "requested", "waiting", + "pending", "action_required", "cancelled", "failure", From 29371e949126db53cc8d93132d23812a06fc72fd Mon Sep 17 00:00:00 2001 From: ChandranshuRao14 Date: Thu, 26 Dec 2024 22:52:32 -0500 Subject: [PATCH 08/50] Extract logic into helper function --- pkg/cmd/repo/edit/edit.go | 82 ++++++++++++++++--------------- pkg/cmd/repo/edit/edit_test.go | 89 ++++++++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 38 deletions(-) diff --git a/pkg/cmd/repo/edit/edit.go b/pkg/cmd/repo/edit/edit.go index 80e460186..8efcea11d 100644 --- a/pkg/cmd/repo/edit/edit.go +++ b/pkg/cmd/repo/edit/edit.go @@ -89,16 +89,6 @@ type EditRepositoryInput struct { Visibility *string `json:"visibility,omitempty"` } -type SecurityAndAnalysisInput struct { - EnableAdvancedSecurity *SecurityAndAnalysisStatus `json:"advanced_security,omitempty"` - EnableSecretScanning *SecurityAndAnalysisStatus `json:"secret_scanning,omitempty"` - EnableSecretScanningPushProtection *SecurityAndAnalysisStatus `json:"secret_scanning_push_protection,omitempty"` -} - -type SecurityAndAnalysisStatus struct { - Status *string `json:"status,omitempty"` -} - func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobra.Command { opts := &EditOptions{ IO: f.IOStreams, @@ -172,6 +162,18 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobr return cmdutil.FlagErrorf("use of --visibility flag requires --accept-visibility-change-consequences flag") } + if hasSecurityEdits(opts.Edits) { + apiClient := api.NewClientFromHTTP(opts.HTTPClient) + repo, err := api.FetchRepository(apiClient, opts.Repository, []string{"viewerCanAdminister"}) + if err != nil { + return err + } + if !repo.ViewerCanAdminister { + return fmt.Errorf("you do not have sufficient permissions to edit repository security and analysis features") + } + opts.Edits.SecurityAndAnalysis = transformSecurityAndAnalysisOpts(opts) + } + if runF != nil { return runF(opts) } @@ -258,34 +260,6 @@ func editRun(ctx context.Context, opts *EditOptions) error { } } - if hasSecurityEdits(opts.Edits) { - apiClient := api.NewClientFromHTTP(opts.HTTPClient) - repo, err := api.FetchRepository(apiClient, opts.Repository, []string{"viewerCanAdminister"}) - if err != nil { - return err - } - if !repo.ViewerCanAdminister { - return fmt.Errorf("you do not have sufficient permissions to edit repository security and analysis features") - } - - opts.Edits.SecurityAndAnalysis = &SecurityAndAnalysisInput{} - if opts.Edits.enableAdvancedSecurity != nil { - opts.Edits.SecurityAndAnalysis.EnableAdvancedSecurity = &SecurityAndAnalysisStatus{ - Status: boolToStatus(*opts.Edits.enableAdvancedSecurity), - } - } - if opts.Edits.enableSecretScanning != nil { - opts.Edits.SecurityAndAnalysis.EnableSecretScanning = &SecurityAndAnalysisStatus{ - Status: boolToStatus(*opts.Edits.enableSecretScanning), - } - } - if opts.Edits.enableSecretScanningPushProtection != nil { - opts.Edits.SecurityAndAnalysis.EnableSecretScanningPushProtection = &SecurityAndAnalysisStatus{ - Status: boolToStatus(*opts.Edits.enableSecretScanningPushProtection), - } - } - } - apiPath := fmt.Sprintf("repos/%s/%s", repo.RepoOwner(), repo.RepoName()) body := &bytes.Buffer{} @@ -620,3 +594,35 @@ func boolToStatus(status bool) *string { func hasSecurityEdits(edits EditRepositoryInput) bool { return edits.enableAdvancedSecurity != nil || edits.enableSecretScanning != nil || edits.enableSecretScanningPushProtection != nil } + +type SecurityAndAnalysisInput struct { + EnableAdvancedSecurity *SecurityAndAnalysisStatus `json:"advanced_security,omitempty"` + EnableSecretScanning *SecurityAndAnalysisStatus `json:"secret_scanning,omitempty"` + EnableSecretScanningPushProtection *SecurityAndAnalysisStatus `json:"secret_scanning_push_protection,omitempty"` +} + +type SecurityAndAnalysisStatus struct { + Status *string `json:"status,omitempty"` +} + +// Transform security and analysis parameters to properly serialize EditRepositoryInput +// See API Docs: https://docs.github.com/en/rest/repos/repos?apiVersion=2022-11-28#update-a-repository +func transformSecurityAndAnalysisOpts(opts *EditOptions) *SecurityAndAnalysisInput { + securityOptions := &SecurityAndAnalysisInput{} + if opts.Edits.enableAdvancedSecurity != nil { + securityOptions.EnableAdvancedSecurity = &SecurityAndAnalysisStatus{ + Status: boolToStatus(*opts.Edits.enableAdvancedSecurity), + } + } + if opts.Edits.enableSecretScanning != nil { + securityOptions.EnableSecretScanning = &SecurityAndAnalysisStatus{ + Status: boolToStatus(*opts.Edits.enableSecretScanning), + } + } + if opts.Edits.enableSecretScanningPushProtection != nil { + securityOptions.EnableSecretScanningPushProtection = &SecurityAndAnalysisStatus{ + Status: boolToStatus(*opts.Edits.enableSecretScanningPushProtection), + } + } + return securityOptions +} diff --git a/pkg/cmd/repo/edit/edit_test.go b/pkg/cmd/repo/edit/edit_test.go index 93d44788d..93b256465 100644 --- a/pkg/cmd/repo/edit/edit_test.go +++ b/pkg/cmd/repo/edit/edit_test.go @@ -700,6 +700,95 @@ func Test_editRun_interactive(t *testing.T) { } } +func Test_transformSecurityAndAnalysisOpts(t *testing.T) { + tests := []struct { + name string + opts EditOptions + want *SecurityAndAnalysisInput + }{ + { + name: "Enable all security and analysis settings", + opts: EditOptions{ + Edits: EditRepositoryInput{ + enableAdvancedSecurity: bp(true), + enableSecretScanning: bp(true), + enableSecretScanningPushProtection: bp(true), + }, + }, + want: &SecurityAndAnalysisInput{ + EnableAdvancedSecurity: &SecurityAndAnalysisStatus{ + Status: sp("enabled"), + }, + EnableSecretScanning: &SecurityAndAnalysisStatus{ + Status: sp("enabled"), + }, + EnableSecretScanningPushProtection: &SecurityAndAnalysisStatus{ + Status: sp("enabled"), + }, + }, + }, + { + name: "Disable all security and analysis settings", + opts: EditOptions{ + Edits: EditRepositoryInput{ + enableAdvancedSecurity: bp(false), + enableSecretScanning: bp(false), + enableSecretScanningPushProtection: bp(false), + }, + }, + want: &SecurityAndAnalysisInput{ + EnableAdvancedSecurity: &SecurityAndAnalysisStatus{ + Status: sp("disabled"), + }, + EnableSecretScanning: &SecurityAndAnalysisStatus{ + Status: sp("disabled"), + }, + EnableSecretScanningPushProtection: &SecurityAndAnalysisStatus{ + Status: sp("disabled"), + }, + }, + }, + { + name: "Enable only advanced security", + opts: EditOptions{ + Edits: EditRepositoryInput{ + enableAdvancedSecurity: bp(true), + }, + }, + want: &SecurityAndAnalysisInput{ + EnableAdvancedSecurity: &SecurityAndAnalysisStatus{ + Status: sp("enabled"), + }, + EnableSecretScanning: nil, + EnableSecretScanningPushProtection: nil, + }, + }, + { + name: "Disable only secret scanning", + opts: EditOptions{ + Edits: EditRepositoryInput{ + enableSecretScanning: bp(false), + }, + }, + want: &SecurityAndAnalysisInput{ + EnableAdvancedSecurity: nil, + EnableSecretScanning: &SecurityAndAnalysisStatus{ + Status: sp("disabled"), + }, + EnableSecretScanningPushProtection: nil, + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + opts := &tt.opts + transformed := transformSecurityAndAnalysisOpts(opts) + assert.Equal(t, tt.want, transformed) + }) + } +} + func sp(v string) *string { return &v } From eed3626521bbfacd758ddba30439e1c006c86f76 Mon Sep 17 00:00:00 2001 From: shauryatiwari1 Date: Fri, 27 Dec 2024 15:15:21 +0530 Subject: [PATCH 09/50] Remove release discussion posts and clean up related block in deployment yml --- .github/workflows/deployment.yml | 2 -- 1 file changed, 2 deletions(-) diff --git a/.github/workflows/deployment.yml b/.github/workflows/deployment.yml index 2dd6af675..a8356272e 100644 --- a/.github/workflows/deployment.yml +++ b/.github/workflows/deployment.yml @@ -351,8 +351,6 @@ jobs: ) if [[ $TAG_NAME == *-* ]]; then release_args+=( --prerelease ) - else - release_args+=( --discussion-category "General" ) fi guard="echo" [ "$DO_PUBLISH" = "false" ] || guard="" From 23c16c9c4cb033840065e0c1d08dbdf6cce7ca3f Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Fri, 20 Dec 2024 13:06:47 -0500 Subject: [PATCH 10/50] Introduce repo autolinks list commands --- pkg/cmd/repo/autolink/autolink.go | 19 +++ pkg/cmd/repo/autolink/http.go | 44 +++++ pkg/cmd/repo/autolink/list.go | 123 ++++++++++++++ pkg/cmd/repo/autolink/list_test.go | 256 +++++++++++++++++++++++++++++ pkg/cmd/repo/autolink/shared.go | 14 ++ pkg/cmd/repo/repo.go | 3 + 6 files changed, 459 insertions(+) create mode 100644 pkg/cmd/repo/autolink/autolink.go create mode 100644 pkg/cmd/repo/autolink/http.go create mode 100644 pkg/cmd/repo/autolink/list.go create mode 100644 pkg/cmd/repo/autolink/list_test.go create mode 100644 pkg/cmd/repo/autolink/shared.go diff --git a/pkg/cmd/repo/autolink/autolink.go b/pkg/cmd/repo/autolink/autolink.go new file mode 100644 index 000000000..1c3c16d44 --- /dev/null +++ b/pkg/cmd/repo/autolink/autolink.go @@ -0,0 +1,19 @@ +package autolink + +import ( + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/spf13/cobra" +) + +func NewCmdAutolink(f *cmdutil.Factory) *cobra.Command { + cmd := &cobra.Command{ + Use: "autolink ", + Short: "Manage autolink references", + Long: "Work with GitHub autolink references.", + } + cmdutil.EnableRepoOverride(cmd, f) + + cmd.AddCommand(newCmdList(f, nil)) + + return cmd +} diff --git a/pkg/cmd/repo/autolink/http.go b/pkg/cmd/repo/autolink/http.go new file mode 100644 index 000000000..b8316a336 --- /dev/null +++ b/pkg/cmd/repo/autolink/http.go @@ -0,0 +1,44 @@ +package autolink + +import ( + "encoding/json" + "fmt" + "io" + "net/http" + + "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/ghinstance" + "github.com/cli/cli/v2/internal/ghrepo" +) + +func repoAutolinks(httpClient *http.Client, repo ghrepo.Interface) ([]autolink, error) { + path := fmt.Sprintf("repos/%s/%s/autolinks", repo.RepoOwner(), repo.RepoName()) + url := ghinstance.RESTPrefix(repo.RepoHost()) + path + req, err := http.NewRequest("GET", url, nil) + if err != nil { + return nil, err + } + + resp, err := httpClient.Do(req) + if err != nil { + return nil, err + } + defer resp.Body.Close() + + if resp.StatusCode > 299 { + return nil, api.HandleHTTPError(resp) + } + + b, err := io.ReadAll(resp.Body) + if err != nil { + return nil, err + } + + var autolinks []autolink + err = json.Unmarshal(b, &autolinks) + if err != nil { + return nil, err + } + + return autolinks, nil +} diff --git a/pkg/cmd/repo/autolink/list.go b/pkg/cmd/repo/autolink/list.go new file mode 100644 index 000000000..45606bfa9 --- /dev/null +++ b/pkg/cmd/repo/autolink/list.go @@ -0,0 +1,123 @@ +package autolink + +import ( + "fmt" + "net/http" + "strconv" + + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/internal/browser" + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/tableprinter" + "github.com/cli/cli/v2/internal/text" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/spf13/cobra" +) + +var autolinkFields = []string{ + "id", + "isAlphanumeric", + "keyPrefix", + "urlTemplate", +} + +type listOptions struct { + BaseRepo func() (ghrepo.Interface, error) + Browser browser.Browser + HTTPClient func() (*http.Client, error) + IO *iostreams.IOStreams + + Exporter cmdutil.Exporter + WebMode bool +} + +func newCmdList(f *cmdutil.Factory, runF func(*listOptions) error) *cobra.Command { + opts := &listOptions{ + Browser: f.Browser, + HTTPClient: f.HttpClient, + IO: f.IOStreams, + } + + cmd := &cobra.Command{ + Use: "list", + Short: "List autolink references for a GitHub repository", + Long: heredoc.Doc(` + Gets all autolink references that are configured for a repository. + + Information about autolinks is only available to repository administrators. + `), + Aliases: []string{"ls"}, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, args []string) error { + opts.BaseRepo = f.BaseRepo + + if runF != nil { + return runF(opts) + } + + return listRun(opts) + }, + } + + cmd.Flags().BoolVarP(&opts.WebMode, "web", "w", false, "List autolink references in the web browser") + cmdutil.AddJSONFlags(cmd, &opts.Exporter, autolinkFields) + + return cmd +} + +func listRun(opts *listOptions) error { + client, err := opts.HTTPClient() + if err != nil { + return err + } + + repo, err := opts.BaseRepo() + if err != nil { + return err + } + + if opts.WebMode { + autolinksListURL := ghrepo.GenerateRepoURL(repo, "settings/key_links") + + if opts.IO.IsStdoutTTY() { + fmt.Fprintf(opts.IO.ErrOut, "Opening %s in your browser.\n", text.DisplayURL(autolinksListURL)) + } + + return opts.Browser.Browse(autolinksListURL) + } + + autolinks, err := repoAutolinks(client, repo) + if err != nil { + return err + } + + if len(autolinks) == 0 { + return cmdutil.NewNoResultsError(fmt.Sprintf("no autolinks found in %s", ghrepo.FullName(repo))) + } + + if opts.Exporter != nil { + return opts.Exporter.Write(opts.IO, autolinks) + } + + if opts.IO.IsStdoutTTY() { + title := listHeader(ghrepo.FullName(repo), len(autolinks)) + fmt.Fprintf(opts.IO.Out, "\n%s\n\n", title) + } + + tp := tableprinter.New(opts.IO, tableprinter.WithHeader("ID", "KEY PREFIX", "URL TEMPLATE", "ALPHANUMERIC")) + + for _, autolink := range autolinks { + tp.AddField(fmt.Sprintf("%d", autolink.ID)) + tp.AddField(autolink.KeyPrefix) + tp.AddField(autolink.URLTemplate) + tp.AddField(strconv.FormatBool(autolink.IsAlphanumeric)) + tp.EndRow() + } + + return tp.Render() +} + +func listHeader(repoName string, count int) string { + return fmt.Sprintf("Showing %s in %s", text.Pluralize(count, "autolink reference"), repoName) +} diff --git a/pkg/cmd/repo/autolink/list_test.go b/pkg/cmd/repo/autolink/list_test.go new file mode 100644 index 000000000..2e3c21374 --- /dev/null +++ b/pkg/cmd/repo/autolink/list_test.go @@ -0,0 +1,256 @@ +package autolink + +import ( + "bytes" + "net/http" + "testing" + + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/internal/browser" + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/google/shlex" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewCmdList(t *testing.T) { + tests := []struct { + name string + input string + output listOptions + wantErr bool + wantExporter bool + errMsg string + }{ + { + name: "no argument", + input: "", + output: listOptions{}, + }, + { + name: "web flag", + input: "--web", + output: listOptions{WebMode: true}, + }, + { + name: "json flag", + input: "--json id", + output: listOptions{}, + wantExporter: true, + }, + { + name: "invalid json flag", + input: "--json invalid", + wantErr: true, + errMsg: heredoc.Doc(` + Unknown JSON field: "invalid" + Available fields: + id + isAlphanumeric + keyPrefix + urlTemplate`), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + f := &cmdutil.Factory{ + IOStreams: ios, + } + + argv, err := shlex.Split(tt.input) + require.NoError(t, err) + + var gotOpts *listOptions + cmd := newCmdList(f, func(opts *listOptions) error { + gotOpts = opts + return nil + }) + + cmd.SetArgs(argv) + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + _, err = cmd.ExecuteC() + if tt.wantErr { + require.EqualError(t, err, tt.errMsg) + return + } + + require.NoError(t, err) + assert.Equal(t, tt.output.WebMode, gotOpts.WebMode) + assert.Equal(t, tt.wantExporter, gotOpts.Exporter != nil) + }) + } +} + +func TestListRun(t *testing.T) { + tests := []struct { + name string + opts *listOptions + isTTY bool + httpStubs func(t *testing.T, reg *httpmock.Registry) + wantStdout string + wantStderr string + wantErr bool + }{ + { + name: "list tty", + opts: &listOptions{}, + isTTY: true, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/autolinks"), + httpmock.StringResponse(`[ + { + "id": 1, + "key_prefix": "TICKET-", + "url_template": "https://example.com/TICKET?query=", + "is_alphanumeric": true + }, + { + "id": 2, + "key_prefix": "STORY-", + "url_template": "https://example.com/STORY?id=", + "is_alphanumeric": false + } + ]`), + ) + }, + wantStdout: heredoc.Doc(` + + Showing 2 autolink references in OWNER/REPO + + ID KEY PREFIX URL TEMPLATE ALPHANUMERIC + 1 TICKET- https://example.com/TICKET?query= true + 2 STORY- https://example.com/STORY?id= false + `), + wantStderr: "", + }, + { + name: "list json", + opts: &listOptions{ + Exporter: func() cmdutil.Exporter { + exporter := cmdutil.NewJSONExporter() + exporter.SetFields([]string{"id"}) + return exporter + }(), + }, + isTTY: true, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/autolinks"), + httpmock.StringResponse(`[ + { + "id": 1, + "key_prefix": "TICKET-", + "url_template": "https://example.com/TICKET?query=", + "is_alphanumeric": true + }, + { + "id": 2, + "key_prefix": "STORY-", + "url_template": "https://example.com/STORY?id=", + "is_alphanumeric": false + } + ]`), + ) + }, + wantStdout: "[{\"id\":1},{\"id\":2}]\n", + wantStderr: "", + }, + { + name: "list non-tty", + opts: &listOptions{}, + isTTY: false, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/autolinks"), + httpmock.StringResponse(`[ + { + "id": 1, + "key_prefix": "TICKET-", + "url_template": "https://example.com/TICKET?query=", + "is_alphanumeric": true + }, + { + "id": 2, + "key_prefix": "STORY-", + "url_template": "https://example.com/STORY?id=", + "is_alphanumeric": false + } + ]`), + ) + }, + wantStdout: heredoc.Doc(` + 1 TICKET- https://example.com/TICKET?query= true + 2 STORY- https://example.com/STORY?id= false + `), + wantStderr: "", + }, + + { + name: "no results", + opts: &listOptions{}, + isTTY: true, + httpStubs: func(t *testing.T, reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/autolinks"), + httpmock.StringResponse(`[]`), + ) + }, + wantStdout: "", + wantStderr: "", + wantErr: true, + }, + { + name: "web mode", + isTTY: true, + opts: &listOptions{WebMode: true}, + wantStderr: "Opening https://github.com/OWNER/REPO/settings/key_links in your browser.\n", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ios, _, stdout, stderr := iostreams.Test() + ios.SetStdoutTTY(tt.isTTY) + ios.SetStdinTTY(tt.isTTY) + ios.SetStderrTTY(tt.isTTY) + + reg := &httpmock.Registry{} + if tt.httpStubs != nil { + tt.httpStubs(t, reg) + } + + defer reg.Verify(t) + + opts := tt.opts + opts.IO = ios + opts.Browser = &browser.Stub{} + + opts.IO = ios + opts.BaseRepo = func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil } + + opts.HTTPClient = func() (*http.Client, error) { return &http.Client{Transport: reg}, nil } + + err := listRun(opts) + if (err != nil) != tt.wantErr { + t.Errorf("listRun() return error: %v", err) + return + } + + if stdout.String() != tt.wantStdout { + t.Errorf("wants stdout %q, got %q", tt.wantStdout, stdout.String()) + } + if stderr.String() != tt.wantStderr { + t.Errorf("wants stderr %q, got %q", tt.wantStderr, stderr.String()) + } + }) + } +} diff --git a/pkg/cmd/repo/autolink/shared.go b/pkg/cmd/repo/autolink/shared.go new file mode 100644 index 000000000..c8478b346 --- /dev/null +++ b/pkg/cmd/repo/autolink/shared.go @@ -0,0 +1,14 @@ +package autolink + +import "github.com/cli/cli/v2/pkg/cmdutil" + +type autolink struct { + ID int `json:"id"` + IsAlphanumeric bool `json:"is_alphanumeric"` + KeyPrefix string `json:"key_prefix"` + URLTemplate string `json:"url_template"` +} + +func (s *autolink) ExportData(fields []string) map[string]interface{} { + return cmdutil.StructExportData(s, fields) +} diff --git a/pkg/cmd/repo/repo.go b/pkg/cmd/repo/repo.go index 14a4bf49c..687cf5a8a 100644 --- a/pkg/cmd/repo/repo.go +++ b/pkg/cmd/repo/repo.go @@ -3,6 +3,7 @@ package repo import ( "github.com/MakeNowJust/heredoc" repoArchiveCmd "github.com/cli/cli/v2/pkg/cmd/repo/archive" + repoAutolinkCmd "github.com/cli/cli/v2/pkg/cmd/repo/autolink" repoCloneCmd "github.com/cli/cli/v2/pkg/cmd/repo/clone" repoCreateCmd "github.com/cli/cli/v2/pkg/cmd/repo/create" creditsCmd "github.com/cli/cli/v2/pkg/cmd/repo/credits" @@ -19,6 +20,7 @@ import ( repoSyncCmd "github.com/cli/cli/v2/pkg/cmd/repo/sync" repoUnarchiveCmd "github.com/cli/cli/v2/pkg/cmd/repo/unarchive" repoViewCmd "github.com/cli/cli/v2/pkg/cmd/repo/view" + "github.com/cli/cli/v2/pkg/cmdutil" "github.com/spf13/cobra" ) @@ -64,6 +66,7 @@ func NewCmdRepo(f *cmdutil.Factory) *cobra.Command { repoDeleteCmd.NewCmdDelete(f, nil), creditsCmd.NewCmdRepoCredits(f, nil), gardenCmd.NewCmdGarden(f, nil), + repoAutolinkCmd.NewCmdAutolink(f), ) return cmd From 5fb98524e0d864dd36e6c6dd6fe25e5568f7fe3a Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Mon, 23 Dec 2024 14:34:21 -0800 Subject: [PATCH 11/50] Apply PR comment changes --- pkg/cmd/repo/autolink/autolink.go | 11 ++++++++++- pkg/cmd/repo/autolink/http.go | 4 +++- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/repo/autolink/autolink.go b/pkg/cmd/repo/autolink/autolink.go index 1c3c16d44..9c0972f7c 100644 --- a/pkg/cmd/repo/autolink/autolink.go +++ b/pkg/cmd/repo/autolink/autolink.go @@ -1,6 +1,7 @@ package autolink import ( + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/spf13/cobra" ) @@ -9,7 +10,15 @@ func NewCmdAutolink(f *cmdutil.Factory) *cobra.Command { cmd := &cobra.Command{ Use: "autolink ", Short: "Manage autolink references", - Long: "Work with GitHub autolink references.", + Long: heredoc.Docf(` + Work with GitHub autolink references. + + GitHub autolinks require admin access to configure and can be found at + https://github.com/{owner}/{repo}/settings/key_links. + Use %[1]sgh repo autolink list --web%[1]s to open this page for the current repository. + + For more information about GitHub autolinks, see https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/managing-repository-settings/configuring-autolinks-to-reference-external-resources + `, "`"), } cmdutil.EnableRepoOverride(cmd, f) diff --git a/pkg/cmd/repo/autolink/http.go b/pkg/cmd/repo/autolink/http.go index b8316a336..d51ac369c 100644 --- a/pkg/cmd/repo/autolink/http.go +++ b/pkg/cmd/repo/autolink/http.go @@ -25,7 +25,9 @@ func repoAutolinks(httpClient *http.Client, repo ghrepo.Interface) ([]autolink, } defer resp.Body.Close() - if resp.StatusCode > 299 { + 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) } From a390ce4f10677c8fd9d87ac49f08e1f05f29a709 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Mon, 23 Dec 2024 15:08:58 -0800 Subject: [PATCH 12/50] Refactor autolink list and test to use http interface for simpler testing 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. --- pkg/cmd/repo/autolink/http.go | 14 ++- pkg/cmd/repo/autolink/list.go | 30 +++--- pkg/cmd/repo/autolink/list_test.go | 158 ++++++++++++++--------------- 3 files changed, 104 insertions(+), 98 deletions(-) diff --git a/pkg/cmd/repo/autolink/http.go b/pkg/cmd/repo/autolink/http.go index d51ac369c..13e8adebe 100644 --- a/pkg/cmd/repo/autolink/http.go +++ b/pkg/cmd/repo/autolink/http.go @@ -11,7 +11,17 @@ import ( "github.com/cli/cli/v2/internal/ghrepo" ) -func repoAutolinks(httpClient *http.Client, repo ghrepo.Interface) ([]autolink, error) { +type AutolinkGetter struct { + HttpClient *http.Client +} + +func NewAutolinkGetter(httpClient *http.Client) *AutolinkGetter { + return &AutolinkGetter{ + HttpClient: httpClient, + } +} + +func (a *AutolinkGetter) Get(repo ghrepo.Interface) ([]autolink, error) { path := fmt.Sprintf("repos/%s/%s/autolinks", repo.RepoOwner(), repo.RepoName()) url := ghinstance.RESTPrefix(repo.RepoHost()) + path req, err := http.NewRequest("GET", url, nil) @@ -19,7 +29,7 @@ func repoAutolinks(httpClient *http.Client, repo ghrepo.Interface) ([]autolink, return nil, err } - resp, err := httpClient.Do(req) + resp, err := a.HttpClient.Do(req) if err != nil { return nil, err } diff --git a/pkg/cmd/repo/autolink/list.go b/pkg/cmd/repo/autolink/list.go index 45606bfa9..41d0d7266 100644 --- a/pkg/cmd/repo/autolink/list.go +++ b/pkg/cmd/repo/autolink/list.go @@ -2,7 +2,6 @@ package autolink import ( "fmt" - "net/http" "strconv" "github.com/MakeNowJust/heredoc" @@ -23,20 +22,23 @@ var autolinkFields = []string{ } type listOptions struct { - BaseRepo func() (ghrepo.Interface, error) - Browser browser.Browser - HTTPClient func() (*http.Client, error) - IO *iostreams.IOStreams + BaseRepo func() (ghrepo.Interface, error) + Browser browser.Browser + AutolinkClient AutolinkClient + IO *iostreams.IOStreams Exporter cmdutil.Exporter WebMode bool } +type AutolinkClient interface { + Get(repo ghrepo.Interface) ([]autolink, error) +} + func newCmdList(f *cmdutil.Factory, runF func(*listOptions) error) *cobra.Command { opts := &listOptions{ - Browser: f.Browser, - HTTPClient: f.HttpClient, - IO: f.IOStreams, + Browser: f.Browser, + IO: f.IOStreams, } cmd := &cobra.Command{ @@ -52,6 +54,12 @@ func newCmdList(f *cmdutil.Factory, runF func(*listOptions) error) *cobra.Comman RunE: func(cmd *cobra.Command, args []string) error { opts.BaseRepo = f.BaseRepo + httpClient, err := f.HttpClient() + if err != nil { + return err + } + opts.AutolinkClient = NewAutolinkGetter(httpClient) + if runF != nil { return runF(opts) } @@ -67,10 +75,6 @@ func newCmdList(f *cmdutil.Factory, runF func(*listOptions) error) *cobra.Comman } func listRun(opts *listOptions) error { - client, err := opts.HTTPClient() - if err != nil { - return err - } repo, err := opts.BaseRepo() if err != nil { @@ -87,7 +91,7 @@ func listRun(opts *listOptions) error { return opts.Browser.Browse(autolinksListURL) } - autolinks, err := repoAutolinks(client, repo) + autolinks, err := opts.AutolinkClient.Get(repo) if err != nil { return err } diff --git a/pkg/cmd/repo/autolink/list_test.go b/pkg/cmd/repo/autolink/list_test.go index 2e3c21374..4a41f9ad2 100644 --- a/pkg/cmd/repo/autolink/list_test.go +++ b/pkg/cmd/repo/autolink/list_test.go @@ -2,6 +2,7 @@ package autolink import ( "bytes" + "fmt" "net/http" "testing" @@ -9,7 +10,6 @@ import ( "github.com/cli/cli/v2/internal/browser" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmdutil" - "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" @@ -61,6 +61,9 @@ func TestNewCmdList(t *testing.T) { f := &cmdutil.Factory{ IOStreams: ios, } + f.HttpClient = func() (*http.Client, error) { + return &http.Client{}, nil + } argv, err := shlex.Split(tt.input) require.NoError(t, err) @@ -89,12 +92,28 @@ func TestNewCmdList(t *testing.T) { } } +type mockAutoLinkGetter struct { + Response []autolink +} + +func (m *mockAutoLinkGetter) Get(repo ghrepo.Interface) ([]autolink, error) { + return m.Response, nil +} + +type mockAutoLinkGetterError struct { + err error +} + +func (me *mockAutoLinkGetterError) Get(repo ghrepo.Interface) ([]autolink, error) { + return nil, me.err +} + func TestListRun(t *testing.T) { tests := []struct { name string opts *listOptions isTTY bool - httpStubs func(t *testing.T, reg *httpmock.Registry) + response []autolink wantStdout string wantStderr string wantErr bool @@ -103,24 +122,19 @@ func TestListRun(t *testing.T) { name: "list tty", opts: &listOptions{}, isTTY: true, - httpStubs: func(t *testing.T, reg *httpmock.Registry) { - reg.Register( - httpmock.REST("GET", "repos/OWNER/REPO/autolinks"), - httpmock.StringResponse(`[ - { - "id": 1, - "key_prefix": "TICKET-", - "url_template": "https://example.com/TICKET?query=", - "is_alphanumeric": true - }, - { - "id": 2, - "key_prefix": "STORY-", - "url_template": "https://example.com/STORY?id=", - "is_alphanumeric": false - } - ]`), - ) + response: []autolink{ + { + ID: 1, + KeyPrefix: "TICKET-", + URLTemplate: "https://example.com/TICKET?query=", + IsAlphanumeric: true, + }, + { + ID: 2, + KeyPrefix: "STORY-", + URLTemplate: "https://example.com/STORY?id=", + IsAlphanumeric: false, + }, }, wantStdout: heredoc.Doc(` @@ -142,24 +156,19 @@ func TestListRun(t *testing.T) { }(), }, isTTY: true, - httpStubs: func(t *testing.T, reg *httpmock.Registry) { - reg.Register( - httpmock.REST("GET", "repos/OWNER/REPO/autolinks"), - httpmock.StringResponse(`[ - { - "id": 1, - "key_prefix": "TICKET-", - "url_template": "https://example.com/TICKET?query=", - "is_alphanumeric": true - }, - { - "id": 2, - "key_prefix": "STORY-", - "url_template": "https://example.com/STORY?id=", - "is_alphanumeric": false - } - ]`), - ) + response: []autolink{ + { + ID: 1, + KeyPrefix: "TICKET-", + URLTemplate: "https://example.com/TICKET?query=", + IsAlphanumeric: true, + }, + { + ID: 2, + KeyPrefix: "STORY-", + URLTemplate: "https://example.com/STORY?id=", + IsAlphanumeric: false, + }, }, wantStdout: "[{\"id\":1},{\"id\":2}]\n", wantStderr: "", @@ -168,24 +177,19 @@ func TestListRun(t *testing.T) { name: "list non-tty", opts: &listOptions{}, isTTY: false, - httpStubs: func(t *testing.T, reg *httpmock.Registry) { - reg.Register( - httpmock.REST("GET", "repos/OWNER/REPO/autolinks"), - httpmock.StringResponse(`[ - { - "id": 1, - "key_prefix": "TICKET-", - "url_template": "https://example.com/TICKET?query=", - "is_alphanumeric": true - }, - { - "id": 2, - "key_prefix": "STORY-", - "url_template": "https://example.com/STORY?id=", - "is_alphanumeric": false - } - ]`), - ) + response: []autolink{ + { + ID: 1, + KeyPrefix: "TICKET-", + URLTemplate: "https://example.com/TICKET?query=", + IsAlphanumeric: true, + }, + { + ID: 2, + KeyPrefix: "STORY-", + URLTemplate: "https://example.com/STORY?id=", + IsAlphanumeric: false, + }, }, wantStdout: heredoc.Doc(` 1 TICKET- https://example.com/TICKET?query= true @@ -195,15 +199,10 @@ func TestListRun(t *testing.T) { }, { - name: "no results", - opts: &listOptions{}, - isTTY: true, - httpStubs: func(t *testing.T, reg *httpmock.Registry) { - reg.Register( - httpmock.REST("GET", "repos/OWNER/REPO/autolinks"), - httpmock.StringResponse(`[]`), - ) - }, + name: "no results", + opts: &listOptions{}, + isTTY: true, + response: []autolink{}, wantStdout: "", wantStderr: "", wantErr: true, @@ -223,13 +222,6 @@ func TestListRun(t *testing.T) { ios.SetStdinTTY(tt.isTTY) ios.SetStderrTTY(tt.isTTY) - reg := &httpmock.Registry{} - if tt.httpStubs != nil { - tt.httpStubs(t, reg) - } - - defer reg.Verify(t) - opts := tt.opts opts.IO = ios opts.Browser = &browser.Stub{} @@ -237,19 +229,19 @@ func TestListRun(t *testing.T) { opts.IO = ios opts.BaseRepo = func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil } - opts.HTTPClient = func() (*http.Client, error) { return &http.Client{Transport: reg}, nil } - - err := listRun(opts) - if (err != nil) != tt.wantErr { - t.Errorf("listRun() return error: %v", err) - return + if tt.wantErr { + opts.AutolinkClient = &mockAutoLinkGetterError{err: fmt.Errorf("mock error")} + err := listRun(opts) + require.Error(t, err) + } else { + opts.AutolinkClient = &mockAutoLinkGetter{Response: tt.response} + err := listRun(opts) + require.NoError(t, err) + assert.Equal(t, tt.wantStdout, stdout.String()) } - if stdout.String() != tt.wantStdout { - t.Errorf("wants stdout %q, got %q", tt.wantStdout, stdout.String()) - } - if stderr.String() != tt.wantStderr { - t.Errorf("wants stderr %q, got %q", tt.wantStderr, stderr.String()) + if tt.wantStderr != "" { + assert.Equal(t, tt.wantStderr, stderr.String()) } }) } From 4a74cc8856f3c0178856c2b5ec7c6afd3df3bc86 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Mon, 23 Dec 2024 15:43:02 -0800 Subject: [PATCH 13/50] Add testing for AutoLinkGetter 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. --- pkg/cmd/repo/autolink/http.go | 2 +- pkg/cmd/repo/autolink/http_test.go | 79 ++++++++++++++++++++++++++++++ 2 files changed, 80 insertions(+), 1 deletion(-) create mode 100644 pkg/cmd/repo/autolink/http_test.go diff --git a/pkg/cmd/repo/autolink/http.go b/pkg/cmd/repo/autolink/http.go index 13e8adebe..a10e98f7b 100644 --- a/pkg/cmd/repo/autolink/http.go +++ b/pkg/cmd/repo/autolink/http.go @@ -36,7 +36,7 @@ func (a *AutolinkGetter) Get(repo ghrepo.Interface) ([]autolink, error) { defer resp.Body.Close() 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) + 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 { return nil, api.HandleHTTPError(resp) } diff --git a/pkg/cmd/repo/autolink/http_test.go b/pkg/cmd/repo/autolink/http_test.go new file mode 100644 index 000000000..64ffc1e40 --- /dev/null +++ b/pkg/cmd/repo/autolink/http_test.go @@ -0,0 +1,79 @@ +package autolink + +import ( + "fmt" + "net/http" + "testing" + + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestNewAutolinkGetter(t *testing.T) { + httpClient := &http.Client{} + autolinkGetter := NewAutolinkGetter(httpClient) + assert.NotNil(t, autolinkGetter) +} + +func TestAutoLinkGetter_Get(t *testing.T) { + tests := []struct { + name string + repo ghrepo.Interface + resp []autolink + status int + }{ + { + name: "no autolinks", + repo: ghrepo.New("OWNER", "REPO"), + resp: []autolink{}, + status: 200, + }, + { + name: "two autolinks", + repo: ghrepo.New("OWNER", "REPO"), + resp: []autolink{ + { + ID: 1, + IsAlphanumeric: true, + KeyPrefix: "key", + URLTemplate: "https://example.com", + }, + { + ID: 2, + IsAlphanumeric: false, + KeyPrefix: "key2", + URLTemplate: "https://example2.com", + }, + }, + status: 200, + }, + { + name: "http error", + repo: ghrepo.New("OWNER", "REPO"), + status: 404, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + reg.Register( + httpmock.REST("GET", fmt.Sprintf("repos/%s/%s/autolinks", tt.repo.RepoOwner(), tt.repo.RepoName())), + httpmock.StatusJSONResponse(tt.status, tt.resp), + ) + defer reg.Verify(t) + + autolinkGetter := NewAutolinkGetter(&http.Client{Transport: reg}) + autolinks, err := autolinkGetter.Get(tt.repo) + if tt.status == 404 { + require.Error(t, err) + assert.Equal(t, "error getting autolinks: HTTP 404: Must have admin rights to Repository. (https://api.github.com/repos/OWNER/REPO/autolinks)", err.Error()) + } else { + require.NoError(t, err) + assert.Equal(t, tt.resp, autolinks) + } + }) + } +} From 869d25193a39e4644311b97bcb6a5a1b33badec0 Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Mon, 23 Dec 2024 20:26:37 -0500 Subject: [PATCH 14/50] Refactor out early return in test code Co-authored-by: Tyler McGoffin --- pkg/cmd/repo/autolink/list_test.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/repo/autolink/list_test.go b/pkg/cmd/repo/autolink/list_test.go index 4a41f9ad2..4b759aaf7 100644 --- a/pkg/cmd/repo/autolink/list_test.go +++ b/pkg/cmd/repo/autolink/list_test.go @@ -82,12 +82,11 @@ func TestNewCmdList(t *testing.T) { _, err = cmd.ExecuteC() if tt.wantErr { require.EqualError(t, err, tt.errMsg) - return - } - - require.NoError(t, err) - assert.Equal(t, tt.output.WebMode, gotOpts.WebMode) - assert.Equal(t, tt.wantExporter, gotOpts.Exporter != nil) + } else { + require.NoError(t, err) + assert.Equal(t, tt.output.WebMode, gotOpts.WebMode) + assert.Equal(t, tt.wantExporter, gotOpts.Exporter != nil) + } }) } } From ea04d2da30767dbd08e4d94f6f47b420f471fbba Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Mon, 23 Dec 2024 20:27:11 -0500 Subject: [PATCH 15/50] Whitespace --- pkg/cmd/repo/autolink/list_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/repo/autolink/list_test.go b/pkg/cmd/repo/autolink/list_test.go index 4b759aaf7..0c24c5ba6 100644 --- a/pkg/cmd/repo/autolink/list_test.go +++ b/pkg/cmd/repo/autolink/list_test.go @@ -83,10 +83,10 @@ func TestNewCmdList(t *testing.T) { if tt.wantErr { require.EqualError(t, err, tt.errMsg) } else { - require.NoError(t, err) - assert.Equal(t, tt.output.WebMode, gotOpts.WebMode) - assert.Equal(t, tt.wantExporter, gotOpts.Exporter != nil) - } + require.NoError(t, err) + assert.Equal(t, tt.output.WebMode, gotOpts.WebMode) + assert.Equal(t, tt.wantExporter, gotOpts.Exporter != nil) + } }) } } From e98ff2ea387b7d18e2b94f124a0d61c46417f565 Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Mon, 23 Dec 2024 21:09:10 -0500 Subject: [PATCH 16/50] Refactor autolink subcommands into their own packages --- pkg/cmd/repo/autolink/autolink.go | 3 ++- pkg/cmd/repo/autolink/{ => list}/http.go | 2 +- pkg/cmd/repo/autolink/{ => list}/http_test.go | 2 +- pkg/cmd/repo/autolink/{ => list}/list.go | 15 +++++++++++++-- pkg/cmd/repo/autolink/{ => list}/list_test.go | 4 ++-- pkg/cmd/repo/autolink/shared.go | 14 -------------- 6 files changed, 19 insertions(+), 21 deletions(-) rename pkg/cmd/repo/autolink/{ => list}/http.go (98%) rename pkg/cmd/repo/autolink/{ => list}/http_test.go (99%) rename pkg/cmd/repo/autolink/{ => list}/list.go (88%) rename pkg/cmd/repo/autolink/{ => list}/list_test.go (98%) delete mode 100644 pkg/cmd/repo/autolink/shared.go diff --git a/pkg/cmd/repo/autolink/autolink.go b/pkg/cmd/repo/autolink/autolink.go index 9c0972f7c..d9430f562 100644 --- a/pkg/cmd/repo/autolink/autolink.go +++ b/pkg/cmd/repo/autolink/autolink.go @@ -2,6 +2,7 @@ package autolink import ( "github.com/MakeNowJust/heredoc" + cmdList "github.com/cli/cli/v2/pkg/cmd/repo/autolink/list" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/spf13/cobra" ) @@ -22,7 +23,7 @@ func NewCmdAutolink(f *cmdutil.Factory) *cobra.Command { } cmdutil.EnableRepoOverride(cmd, f) - cmd.AddCommand(newCmdList(f, nil)) + cmd.AddCommand(cmdList.NewCmdList(f, nil)) return cmd } diff --git a/pkg/cmd/repo/autolink/http.go b/pkg/cmd/repo/autolink/list/http.go similarity index 98% rename from pkg/cmd/repo/autolink/http.go rename to pkg/cmd/repo/autolink/list/http.go index a10e98f7b..e3fb27e7b 100644 --- a/pkg/cmd/repo/autolink/http.go +++ b/pkg/cmd/repo/autolink/list/http.go @@ -1,4 +1,4 @@ -package autolink +package list import ( "encoding/json" diff --git a/pkg/cmd/repo/autolink/http_test.go b/pkg/cmd/repo/autolink/list/http_test.go similarity index 99% rename from pkg/cmd/repo/autolink/http_test.go rename to pkg/cmd/repo/autolink/list/http_test.go index 64ffc1e40..8a2d5c296 100644 --- a/pkg/cmd/repo/autolink/http_test.go +++ b/pkg/cmd/repo/autolink/list/http_test.go @@ -1,4 +1,4 @@ -package autolink +package list import ( "fmt" diff --git a/pkg/cmd/repo/autolink/list.go b/pkg/cmd/repo/autolink/list/list.go similarity index 88% rename from pkg/cmd/repo/autolink/list.go rename to pkg/cmd/repo/autolink/list/list.go index 41d0d7266..9db044668 100644 --- a/pkg/cmd/repo/autolink/list.go +++ b/pkg/cmd/repo/autolink/list/list.go @@ -1,4 +1,4 @@ -package autolink +package list import ( "fmt" @@ -21,6 +21,17 @@ var autolinkFields = []string{ "urlTemplate", } +type autolink struct { + ID int `json:"id"` + IsAlphanumeric bool `json:"is_alphanumeric"` + KeyPrefix string `json:"key_prefix"` + URLTemplate string `json:"url_template"` +} + +func (s *autolink) ExportData(fields []string) map[string]interface{} { + return cmdutil.StructExportData(s, fields) +} + type listOptions struct { BaseRepo func() (ghrepo.Interface, error) Browser browser.Browser @@ -35,7 +46,7 @@ type AutolinkClient interface { Get(repo ghrepo.Interface) ([]autolink, error) } -func newCmdList(f *cmdutil.Factory, runF func(*listOptions) error) *cobra.Command { +func NewCmdList(f *cmdutil.Factory, runF func(*listOptions) error) *cobra.Command { opts := &listOptions{ Browser: f.Browser, IO: f.IOStreams, diff --git a/pkg/cmd/repo/autolink/list_test.go b/pkg/cmd/repo/autolink/list/list_test.go similarity index 98% rename from pkg/cmd/repo/autolink/list_test.go rename to pkg/cmd/repo/autolink/list/list_test.go index 0c24c5ba6..5db767032 100644 --- a/pkg/cmd/repo/autolink/list_test.go +++ b/pkg/cmd/repo/autolink/list/list_test.go @@ -1,4 +1,4 @@ -package autolink +package list import ( "bytes" @@ -69,7 +69,7 @@ func TestNewCmdList(t *testing.T) { require.NoError(t, err) var gotOpts *listOptions - cmd := newCmdList(f, func(opts *listOptions) error { + cmd := NewCmdList(f, func(opts *listOptions) error { gotOpts = opts return nil }) diff --git a/pkg/cmd/repo/autolink/shared.go b/pkg/cmd/repo/autolink/shared.go deleted file mode 100644 index c8478b346..000000000 --- a/pkg/cmd/repo/autolink/shared.go +++ /dev/null @@ -1,14 +0,0 @@ -package autolink - -import "github.com/cli/cli/v2/pkg/cmdutil" - -type autolink struct { - ID int `json:"id"` - IsAlphanumeric bool `json:"is_alphanumeric"` - KeyPrefix string `json:"key_prefix"` - URLTemplate string `json:"url_template"` -} - -func (s *autolink) ExportData(fields []string) map[string]interface{} { - return cmdutil.StructExportData(s, fields) -} From 67266e9cb883629a0cbdd21b630497b318c8c6ad Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Fri, 27 Dec 2024 21:40:52 -0500 Subject: [PATCH 17/50] PR nits --- pkg/cmd/repo/autolink/list/http.go | 4 ++-- pkg/cmd/repo/autolink/list/http_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/repo/autolink/list/http.go b/pkg/cmd/repo/autolink/list/http.go index e3fb27e7b..269775883 100644 --- a/pkg/cmd/repo/autolink/list/http.go +++ b/pkg/cmd/repo/autolink/list/http.go @@ -35,8 +35,8 @@ func (a *AutolinkGetter) Get(repo ghrepo.Interface) ([]autolink, error) { } defer resp.Body.Close() - 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) + if resp.StatusCode == http.StatusNotFound { + return nil, fmt.Errorf("error getting autolinks: HTTP 404: Perhaps you are missing admin rights to the repository? (https://api.github.com/%s)", path) } else if resp.StatusCode > 299 { return nil, api.HandleHTTPError(resp) } diff --git a/pkg/cmd/repo/autolink/list/http_test.go b/pkg/cmd/repo/autolink/list/http_test.go index 8a2d5c296..77923ed11 100644 --- a/pkg/cmd/repo/autolink/list/http_test.go +++ b/pkg/cmd/repo/autolink/list/http_test.go @@ -69,7 +69,7 @@ func TestAutoLinkGetter_Get(t *testing.T) { autolinks, err := autolinkGetter.Get(tt.repo) if tt.status == 404 { require.Error(t, err) - assert.Equal(t, "error getting autolinks: HTTP 404: Must have admin rights to Repository. (https://api.github.com/repos/OWNER/REPO/autolinks)", err.Error()) + assert.Equal(t, "error getting autolinks: HTTP 404: Perhaps you are missing admin rights to the repository? (https://api.github.com/repos/OWNER/REPO/autolinks)", err.Error()) } else { require.NoError(t, err) assert.Equal(t, tt.resp, autolinks) From cc2428983228b4ae4f1fd522a13cbe0cd891cc01 Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Fri, 27 Dec 2024 21:43:47 -0500 Subject: [PATCH 18/50] Break out autolink list json fields test --- pkg/cmd/repo/autolink/list/list_test.go | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/pkg/cmd/repo/autolink/list/list_test.go b/pkg/cmd/repo/autolink/list/list_test.go index 5db767032..0d9562d6d 100644 --- a/pkg/cmd/repo/autolink/list/list_test.go +++ b/pkg/cmd/repo/autolink/list/list_test.go @@ -11,11 +11,21 @@ import ( "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" + "github.com/cli/cli/v2/pkg/jsonfieldstest" "github.com/google/shlex" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestJSONFields(t *testing.T) { + jsonfieldstest.ExpectCommandToSupportJSONFields(t, NewCmdList, []string{ + "id", + "isAlphanumeric", + "keyPrefix", + "urlTemplate", + }) +} + func TestNewCmdList(t *testing.T) { tests := []struct { name string @@ -41,18 +51,6 @@ func TestNewCmdList(t *testing.T) { output: listOptions{}, wantExporter: true, }, - { - name: "invalid json flag", - input: "--json invalid", - wantErr: true, - errMsg: heredoc.Doc(` - Unknown JSON field: "invalid" - Available fields: - id - isAlphanumeric - keyPrefix - urlTemplate`), - }, } for _, tt := range tests { From dc6320f7f7e274f44de8e458e1eb808f851240c9 Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Fri, 27 Dec 2024 21:47:27 -0500 Subject: [PATCH 19/50] Remove NewAutolinkClient --- pkg/cmd/repo/autolink/list/http.go | 10 ++-------- pkg/cmd/repo/autolink/list/http_test.go | 6 ++++-- pkg/cmd/repo/autolink/list/list.go | 2 +- 3 files changed, 7 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/repo/autolink/list/http.go b/pkg/cmd/repo/autolink/list/http.go index 269775883..8ce2ba166 100644 --- a/pkg/cmd/repo/autolink/list/http.go +++ b/pkg/cmd/repo/autolink/list/http.go @@ -12,13 +12,7 @@ import ( ) type AutolinkGetter struct { - HttpClient *http.Client -} - -func NewAutolinkGetter(httpClient *http.Client) *AutolinkGetter { - return &AutolinkGetter{ - HttpClient: httpClient, - } + HTTPClient *http.Client } func (a *AutolinkGetter) Get(repo ghrepo.Interface) ([]autolink, error) { @@ -29,7 +23,7 @@ func (a *AutolinkGetter) Get(repo ghrepo.Interface) ([]autolink, error) { return nil, err } - resp, err := a.HttpClient.Do(req) + resp, err := a.HTTPClient.Do(req) if err != nil { return nil, err } diff --git a/pkg/cmd/repo/autolink/list/http_test.go b/pkg/cmd/repo/autolink/list/http_test.go index 77923ed11..f75d7e473 100644 --- a/pkg/cmd/repo/autolink/list/http_test.go +++ b/pkg/cmd/repo/autolink/list/http_test.go @@ -13,7 +13,7 @@ import ( func TestNewAutolinkGetter(t *testing.T) { httpClient := &http.Client{} - autolinkGetter := NewAutolinkGetter(httpClient) + autolinkGetter := &AutolinkGetter{HTTPClient: httpClient} assert.NotNil(t, autolinkGetter) } @@ -65,7 +65,9 @@ func TestAutoLinkGetter_Get(t *testing.T) { ) defer reg.Verify(t) - autolinkGetter := NewAutolinkGetter(&http.Client{Transport: reg}) + autolinkGetter := &AutolinkGetter{ + HTTPClient: &http.Client{Transport: reg}, + } autolinks, err := autolinkGetter.Get(tt.repo) if tt.status == 404 { require.Error(t, err) diff --git a/pkg/cmd/repo/autolink/list/list.go b/pkg/cmd/repo/autolink/list/list.go index 9db044668..90c652044 100644 --- a/pkg/cmd/repo/autolink/list/list.go +++ b/pkg/cmd/repo/autolink/list/list.go @@ -69,7 +69,7 @@ func NewCmdList(f *cmdutil.Factory, runF func(*listOptions) error) *cobra.Comman if err != nil { return err } - opts.AutolinkClient = NewAutolinkGetter(httpClient) + opts.AutolinkClient = &AutolinkGetter{HTTPClient: httpClient} if runF != nil { return runF(opts) From 63488a1a06d1426c0146c14f0d67ccefdc32a982 Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Fri, 27 Dec 2024 22:02:19 -0500 Subject: [PATCH 20/50] Use 'list' instead of 'get' for autolink list type and method --- pkg/cmd/repo/autolink/list/http.go | 4 ++-- pkg/cmd/repo/autolink/list/http_test.go | 6 +++--- pkg/cmd/repo/autolink/list/list.go | 6 +++--- pkg/cmd/repo/autolink/list/list_test.go | 4 ++-- 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/cmd/repo/autolink/list/http.go b/pkg/cmd/repo/autolink/list/http.go index 8ce2ba166..b5de22140 100644 --- a/pkg/cmd/repo/autolink/list/http.go +++ b/pkg/cmd/repo/autolink/list/http.go @@ -11,11 +11,11 @@ import ( "github.com/cli/cli/v2/internal/ghrepo" ) -type AutolinkGetter struct { +type AutolinkLister struct { HTTPClient *http.Client } -func (a *AutolinkGetter) Get(repo ghrepo.Interface) ([]autolink, error) { +func (a *AutolinkLister) List(repo ghrepo.Interface) ([]autolink, error) { path := fmt.Sprintf("repos/%s/%s/autolinks", repo.RepoOwner(), repo.RepoName()) url := ghinstance.RESTPrefix(repo.RepoHost()) + path req, err := http.NewRequest("GET", url, nil) diff --git a/pkg/cmd/repo/autolink/list/http_test.go b/pkg/cmd/repo/autolink/list/http_test.go index f75d7e473..eec74cf38 100644 --- a/pkg/cmd/repo/autolink/list/http_test.go +++ b/pkg/cmd/repo/autolink/list/http_test.go @@ -13,7 +13,7 @@ import ( func TestNewAutolinkGetter(t *testing.T) { httpClient := &http.Client{} - autolinkGetter := &AutolinkGetter{HTTPClient: httpClient} + autolinkGetter := &AutolinkLister{HTTPClient: httpClient} assert.NotNil(t, autolinkGetter) } @@ -65,10 +65,10 @@ func TestAutoLinkGetter_Get(t *testing.T) { ) defer reg.Verify(t) - autolinkGetter := &AutolinkGetter{ + autolinkGetter := &AutolinkLister{ HTTPClient: &http.Client{Transport: reg}, } - autolinks, err := autolinkGetter.Get(tt.repo) + autolinks, err := autolinkGetter.List(tt.repo) if tt.status == 404 { require.Error(t, err) assert.Equal(t, "error getting autolinks: HTTP 404: Perhaps you are missing admin rights to the repository? (https://api.github.com/repos/OWNER/REPO/autolinks)", err.Error()) diff --git a/pkg/cmd/repo/autolink/list/list.go b/pkg/cmd/repo/autolink/list/list.go index 90c652044..6f1240605 100644 --- a/pkg/cmd/repo/autolink/list/list.go +++ b/pkg/cmd/repo/autolink/list/list.go @@ -43,7 +43,7 @@ type listOptions struct { } type AutolinkClient interface { - Get(repo ghrepo.Interface) ([]autolink, error) + List(repo ghrepo.Interface) ([]autolink, error) } func NewCmdList(f *cmdutil.Factory, runF func(*listOptions) error) *cobra.Command { @@ -69,7 +69,7 @@ func NewCmdList(f *cmdutil.Factory, runF func(*listOptions) error) *cobra.Comman if err != nil { return err } - opts.AutolinkClient = &AutolinkGetter{HTTPClient: httpClient} + opts.AutolinkClient = &AutolinkLister{HTTPClient: httpClient} if runF != nil { return runF(opts) @@ -102,7 +102,7 @@ func listRun(opts *listOptions) error { return opts.Browser.Browse(autolinksListURL) } - autolinks, err := opts.AutolinkClient.Get(repo) + autolinks, err := opts.AutolinkClient.List(repo) if err != nil { return err } diff --git a/pkg/cmd/repo/autolink/list/list_test.go b/pkg/cmd/repo/autolink/list/list_test.go index 0d9562d6d..d6ed6c9cf 100644 --- a/pkg/cmd/repo/autolink/list/list_test.go +++ b/pkg/cmd/repo/autolink/list/list_test.go @@ -93,7 +93,7 @@ type mockAutoLinkGetter struct { Response []autolink } -func (m *mockAutoLinkGetter) Get(repo ghrepo.Interface) ([]autolink, error) { +func (m *mockAutoLinkGetter) List(repo ghrepo.Interface) ([]autolink, error) { return m.Response, nil } @@ -101,7 +101,7 @@ type mockAutoLinkGetterError struct { err error } -func (me *mockAutoLinkGetterError) Get(repo ghrepo.Interface) ([]autolink, error) { +func (me *mockAutoLinkGetterError) List(repo ghrepo.Interface) ([]autolink, error) { return nil, me.err } From 20f086549adff9b437c2de5ab118010da0af5dd2 Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Fri, 27 Dec 2024 22:03:25 -0500 Subject: [PATCH 21/50] Decode instead of unmarshal --- pkg/cmd/repo/autolink/list/http.go | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/pkg/cmd/repo/autolink/list/http.go b/pkg/cmd/repo/autolink/list/http.go index b5de22140..70d913d70 100644 --- a/pkg/cmd/repo/autolink/list/http.go +++ b/pkg/cmd/repo/autolink/list/http.go @@ -3,7 +3,6 @@ package list import ( "encoding/json" "fmt" - "io" "net/http" "github.com/cli/cli/v2/api" @@ -34,14 +33,8 @@ func (a *AutolinkLister) List(repo ghrepo.Interface) ([]autolink, error) { } else if resp.StatusCode > 299 { return nil, api.HandleHTTPError(resp) } - - b, err := io.ReadAll(resp.Body) - if err != nil { - return nil, err - } - var autolinks []autolink - err = json.Unmarshal(b, &autolinks) + err = json.NewDecoder(resp.Body).Decode(&autolinks) if err != nil { return nil, err } From da826db34227bb8aeb8d7e273a9abafcb6e7e748 Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Fri, 27 Dec 2024 22:58:12 -0500 Subject: [PATCH 22/50] Better error testing for autolink TestListRun --- pkg/cmd/repo/autolink/list/list.go | 1 - pkg/cmd/repo/autolink/list/list_test.go | 170 ++++++++++++++---------- 2 files changed, 97 insertions(+), 74 deletions(-) diff --git a/pkg/cmd/repo/autolink/list/list.go b/pkg/cmd/repo/autolink/list/list.go index 6f1240605..d8a9c9f12 100644 --- a/pkg/cmd/repo/autolink/list/list.go +++ b/pkg/cmd/repo/autolink/list/list.go @@ -86,7 +86,6 @@ func NewCmdList(f *cmdutil.Factory, runF func(*listOptions) error) *cobra.Comman } func listRun(opts *listOptions) error { - repo, err := opts.BaseRepo() if err != nil { return err diff --git a/pkg/cmd/repo/autolink/list/list_test.go b/pkg/cmd/repo/autolink/list/list_test.go index d6ed6c9cf..e6d0603ef 100644 --- a/pkg/cmd/repo/autolink/list/list_test.go +++ b/pkg/cmd/repo/autolink/list/list_test.go @@ -2,7 +2,6 @@ package list import ( "bytes" - "fmt" "net/http" "testing" @@ -51,6 +50,13 @@ func TestNewCmdList(t *testing.T) { output: listOptions{}, wantExporter: true, }, + { + name: "invalid json flag", + input: "--json invalid", + output: listOptions{}, + wantErr: true, + errMsg: "Unknown JSON field: \"invalid\"\nAvailable fields:\n id\n isAlphanumeric\n keyPrefix\n urlTemplate", + }, } for _, tt := range tests { @@ -89,48 +95,34 @@ func TestNewCmdList(t *testing.T) { } } -type mockAutoLinkGetter struct { - Response []autolink -} - -func (m *mockAutoLinkGetter) List(repo ghrepo.Interface) ([]autolink, error) { - return m.Response, nil -} - -type mockAutoLinkGetterError struct { - err error -} - -func (me *mockAutoLinkGetterError) List(repo ghrepo.Interface) ([]autolink, error) { - return nil, me.err -} - func TestListRun(t *testing.T) { tests := []struct { - name string - opts *listOptions - isTTY bool - response []autolink - wantStdout string - wantStderr string - wantErr bool + name string + opts *listOptions + isTTY bool + stubLister stubAutoLinkLister + expectedErr error + wantStdout string + wantStderr string }{ { name: "list tty", opts: &listOptions{}, isTTY: true, - response: []autolink{ - { - ID: 1, - KeyPrefix: "TICKET-", - URLTemplate: "https://example.com/TICKET?query=", - IsAlphanumeric: true, - }, - { - ID: 2, - KeyPrefix: "STORY-", - URLTemplate: "https://example.com/STORY?id=", - IsAlphanumeric: false, + stubLister: stubAutoLinkLister{ + autolinks: []autolink{ + { + ID: 1, + KeyPrefix: "TICKET-", + URLTemplate: "https://example.com/TICKET?query=", + IsAlphanumeric: true, + }, + { + ID: 2, + KeyPrefix: "STORY-", + URLTemplate: "https://example.com/STORY?id=", + IsAlphanumeric: false, + }, }, }, wantStdout: heredoc.Doc(` @@ -153,18 +145,20 @@ func TestListRun(t *testing.T) { }(), }, isTTY: true, - response: []autolink{ - { - ID: 1, - KeyPrefix: "TICKET-", - URLTemplate: "https://example.com/TICKET?query=", - IsAlphanumeric: true, - }, - { - ID: 2, - KeyPrefix: "STORY-", - URLTemplate: "https://example.com/STORY?id=", - IsAlphanumeric: false, + stubLister: stubAutoLinkLister{ + autolinks: []autolink{ + { + ID: 1, + KeyPrefix: "TICKET-", + URLTemplate: "https://example.com/TICKET?query=", + IsAlphanumeric: true, + }, + { + ID: 2, + KeyPrefix: "STORY-", + URLTemplate: "https://example.com/STORY?id=", + IsAlphanumeric: false, + }, }, }, wantStdout: "[{\"id\":1},{\"id\":2}]\n", @@ -174,18 +168,20 @@ func TestListRun(t *testing.T) { name: "list non-tty", opts: &listOptions{}, isTTY: false, - response: []autolink{ - { - ID: 1, - KeyPrefix: "TICKET-", - URLTemplate: "https://example.com/TICKET?query=", - IsAlphanumeric: true, - }, - { - ID: 2, - KeyPrefix: "STORY-", - URLTemplate: "https://example.com/STORY?id=", - IsAlphanumeric: false, + stubLister: stubAutoLinkLister{ + autolinks: []autolink{ + { + ID: 1, + KeyPrefix: "TICKET-", + URLTemplate: "https://example.com/TICKET?query=", + IsAlphanumeric: true, + }, + { + ID: 2, + KeyPrefix: "STORY-", + URLTemplate: "https://example.com/STORY?id=", + IsAlphanumeric: false, + }, }, }, wantStdout: heredoc.Doc(` @@ -194,15 +190,26 @@ func TestListRun(t *testing.T) { `), wantStderr: "", }, - { - name: "no results", - opts: &listOptions{}, - isTTY: true, - response: []autolink{}, - wantStdout: "", - wantStderr: "", - wantErr: true, + name: "no results", + opts: &listOptions{}, + isTTY: true, + stubLister: stubAutoLinkLister{ + autolinks: []autolink{}, + }, + expectedErr: cmdutil.NewNoResultsError("no autolinks found in OWNER/REPO"), + wantStderr: "", + }, + { + name: "client error", + opts: &listOptions{}, + isTTY: true, + stubLister: stubAutoLinkLister{ + autolinks: []autolink{}, + err: testAutolinkClientListError{}, + }, + expectedErr: testAutolinkClientListError{}, + wantStderr: "", }, { name: "web mode", @@ -226,13 +233,13 @@ func TestListRun(t *testing.T) { opts.IO = ios opts.BaseRepo = func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil } - if tt.wantErr { - opts.AutolinkClient = &mockAutoLinkGetterError{err: fmt.Errorf("mock error")} - err := listRun(opts) + opts.AutolinkClient = &tt.stubLister + err := listRun(opts) + + if tt.expectedErr != nil { require.Error(t, err) + require.ErrorIs(t, err, tt.expectedErr) } else { - opts.AutolinkClient = &mockAutoLinkGetter{Response: tt.response} - err := listRun(opts) require.NoError(t, err) assert.Equal(t, tt.wantStdout, stdout.String()) } @@ -243,3 +250,20 @@ func TestListRun(t *testing.T) { }) } } + +type ( + testAutolinkClientListError struct{} + + stubAutoLinkLister struct { + autolinks []autolink + err error + } +) + +func (g stubAutoLinkLister) List(repo ghrepo.Interface) ([]autolink, error) { + return g.autolinks, g.err +} + +func (e testAutolinkClientListError) Error() string { + return "autolink client list error" +} From fa254ba205bd8253178ffec50f89ad405aa969d7 Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Sat, 28 Dec 2024 07:51:47 -0500 Subject: [PATCH 23/50] Complete get -> list renaming --- pkg/cmd/repo/autolink/list/http_test.go | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/pkg/cmd/repo/autolink/list/http_test.go b/pkg/cmd/repo/autolink/list/http_test.go index eec74cf38..fc1e44b23 100644 --- a/pkg/cmd/repo/autolink/list/http_test.go +++ b/pkg/cmd/repo/autolink/list/http_test.go @@ -11,13 +11,7 @@ import ( "github.com/stretchr/testify/require" ) -func TestNewAutolinkGetter(t *testing.T) { - httpClient := &http.Client{} - autolinkGetter := &AutolinkLister{HTTPClient: httpClient} - assert.NotNil(t, autolinkGetter) -} - -func TestAutoLinkGetter_Get(t *testing.T) { +func TestAutoLinkLister_List(t *testing.T) { tests := []struct { name string repo ghrepo.Interface @@ -65,10 +59,10 @@ func TestAutoLinkGetter_Get(t *testing.T) { ) defer reg.Verify(t) - autolinkGetter := &AutolinkLister{ + autolinkLister := &AutolinkLister{ HTTPClient: &http.Client{Transport: reg}, } - autolinks, err := autolinkGetter.List(tt.repo) + autolinks, err := autolinkLister.List(tt.repo) if tt.status == 404 { require.Error(t, err) assert.Equal(t, "error getting autolinks: HTTP 404: Perhaps you are missing admin rights to the repository? (https://api.github.com/repos/OWNER/REPO/autolinks)", err.Error()) From 079719f9230cd7ccdc26c23375016272a5d13ac3 Mon Sep 17 00:00:00 2001 From: ChandranshuRao14 Date: Tue, 31 Dec 2024 00:47:05 -0500 Subject: [PATCH 24/50] Move api call to editRun --- pkg/cmd/repo/edit/edit.go | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/pkg/cmd/repo/edit/edit.go b/pkg/cmd/repo/edit/edit.go index 8efcea11d..cacc9420d 100644 --- a/pkg/cmd/repo/edit/edit.go +++ b/pkg/cmd/repo/edit/edit.go @@ -163,14 +163,6 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(options *EditOptions) error) *cobr } if hasSecurityEdits(opts.Edits) { - apiClient := api.NewClientFromHTTP(opts.HTTPClient) - repo, err := api.FetchRepository(apiClient, opts.Repository, []string{"viewerCanAdminister"}) - if err != nil { - return err - } - if !repo.ViewerCanAdminister { - return fmt.Errorf("you do not have sufficient permissions to edit repository security and analysis features") - } opts.Edits.SecurityAndAnalysis = transformSecurityAndAnalysisOpts(opts) } @@ -260,6 +252,17 @@ func editRun(ctx context.Context, opts *EditOptions) error { } } + if hasSecurityEdits(opts.Edits) { + apiClient := api.NewClientFromHTTP(opts.HTTPClient) + repo, err := api.FetchRepository(apiClient, opts.Repository, []string{"viewerCanAdminister"}) + if err != nil { + return err + } + if !repo.ViewerCanAdminister { + return fmt.Errorf("you do not have sufficient permissions to edit repository security and analysis features") + } + } + apiPath := fmt.Sprintf("repos/%s/%s", repo.RepoOwner(), repo.RepoName()) body := &bytes.Buffer{} From 8560c24f3fbbd828a07c4fef52ddd6e3bc5abf98 Mon Sep 17 00:00:00 2001 From: Aryan Bhosale <36108149+aryanbhosale@users.noreply.github.com> Date: Wed, 1 Jan 2025 11:56:18 +0530 Subject: [PATCH 25/50] fix(repo fork): add non-TTY output when fork is newly created --- pkg/cmd/repo/fork/fork.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/cmd/repo/fork/fork.go b/pkg/cmd/repo/fork/fork.go index 73e269500..971adcb41 100644 --- a/pkg/cmd/repo/fork/fork.go +++ b/pkg/cmd/repo/fork/fork.go @@ -221,6 +221,8 @@ func forkRun(opts *ForkOptions) error { } else { if connectedToTerminal { fmt.Fprintf(stderr, "%s Created fork %s\n", cs.SuccessIconWithColor(cs.Green), cs.Bold(ghrepo.FullName(forkedRepo))) + } else { + fmt.Fprintf(stderr, "Created fork %s\n", ghrepo.FullName(forkedRepo)) } } From 9558d5b60b190fabb08521518b71bb0a954f0a26 Mon Sep 17 00:00:00 2001 From: nobe4 Date: Thu, 2 Jan 2025 16:53:44 +0100 Subject: [PATCH 26/50] docs(repo): make explicit which branch is used when creating a repo This adds a line of documentation in the `gh repo create` command's help specifying which branch for the new repo is selected. --- pkg/cmd/repo/create/create.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index ec7026759..6e813fe33 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -99,6 +99,8 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co For language or platform .gitignore templates to use with %[1]s--gitignore%[1]s, . For license keywords to use with %[1]s--license%[1]s, run %[1]sgh repo license list%[1]s or visit . + + The repo is created with the configured repository default branch, see . `, "`"), Example: heredoc.Doc(` # create a repository interactively From f1c3619003b4f22b84ea6a9df4149a906695af0a Mon Sep 17 00:00:00 2001 From: nobe4 Date: Thu, 2 Jan 2025 17:11:59 +0100 Subject: [PATCH 27/50] Update pkg/cmd/repo/create/create.go Co-authored-by: Kynan Ware <47394200+BagToad@users.noreply.github.com> --- pkg/cmd/repo/create/create.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index 6e813fe33..ba33156c8 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -100,7 +100,7 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co For license keywords to use with %[1]s--license%[1]s, run %[1]sgh repo license list%[1]s or visit . - The repo is created with the configured repository default branch, see . + The repo is created with the configured repository default branch, see . `, "`"), Example: heredoc.Doc(` # create a repository interactively From 375dbf19da99c17b6ce6a130794c4719de5c416d Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Thu, 2 Jan 2025 10:08:28 -0800 Subject: [PATCH 28/50] Add mention of classic token in gh auth login docs --- pkg/cmd/auth/login/login.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go index 8f35972c6..58cbe9038 100644 --- a/pkg/cmd/auth/login/login.go +++ b/pkg/cmd/auth/login/login.go @@ -63,7 +63,7 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm flag. The default authentication mode is a web-based browser flow. After completion, an - authentication token will be stored securely in the system credential store. + authentication token (classic) will be stored securely in the system credential store. If a credential store is not found or there is an issue using it gh will fallback to writing the token to a plain text file. See %[1]sgh auth status%[1]s for its stored location. From a5cf3751cde5278fa76a5ff6c4d2a38a97a21dd4 Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Thu, 2 Jan 2025 13:14:23 -0500 Subject: [PATCH 29/50] Separate type decrarations --- pkg/cmd/repo/autolink/list/list_test.go | 32 ++++++++++++------------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/pkg/cmd/repo/autolink/list/list_test.go b/pkg/cmd/repo/autolink/list/list_test.go index e6d0603ef..3fc8e0261 100644 --- a/pkg/cmd/repo/autolink/list/list_test.go +++ b/pkg/cmd/repo/autolink/list/list_test.go @@ -95,6 +95,21 @@ func TestNewCmdList(t *testing.T) { } } +type stubAutoLinkLister struct { + autolinks []autolink + err error +} + +func (g stubAutoLinkLister) List(repo ghrepo.Interface) ([]autolink, error) { + return g.autolinks, g.err +} + +type testAutolinkClientListError struct{} + +func (e testAutolinkClientListError) Error() string { + return "autolink client list error" +} + func TestListRun(t *testing.T) { tests := []struct { name string @@ -250,20 +265,3 @@ func TestListRun(t *testing.T) { }) } } - -type ( - testAutolinkClientListError struct{} - - stubAutoLinkLister struct { - autolinks []autolink - err error - } -) - -func (g stubAutoLinkLister) List(repo ghrepo.Interface) ([]autolink, error) { - return g.autolinks, g.err -} - -func (e testAutolinkClientListError) Error() string { - return "autolink client list error" -} From ae9e68b80356f91e70fda69ee784efbe08ba78d5 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Thu, 2 Jan 2025 10:41:25 -0800 Subject: [PATCH 30/50] Move mention of classic token to correct line --- pkg/cmd/auth/login/login.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go index 58cbe9038..225c6ed06 100644 --- a/pkg/cmd/auth/login/login.go +++ b/pkg/cmd/auth/login/login.go @@ -63,14 +63,16 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm flag. The default authentication mode is a web-based browser flow. After completion, an - authentication token (classic) will be stored securely in the system credential store. + authentication token will be stored securely in the system credential store. If a credential store is not found or there is an issue using it gh will fallback to writing the token to a plain text file. See %[1]sgh auth status%[1]s for its stored location. - Alternatively, use %[1]s--with-token%[1]s to pass in a token on standard input. + Alternatively, use %[1]s--with-token%[1]s to pass in a token (classic) on standard input. The minimum required scopes for the token are: %[1]srepo%[1]s, %[1]sread:org%[1]s, and %[1]sgist%[1]s. + Fine-grained tokens are not supported. + Alternatively, gh will use the authentication token found in environment variables. This method is most suitable for "headless" use of gh such as in automation. See %[1]sgh help environment%[1]s for more info. From aa793f1dac44bd4cfb4b209932993a7288476525 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Thu, 2 Jan 2025 14:22:20 -0800 Subject: [PATCH 31/50] Update pkg/cmd/auth/login/login.go Co-authored-by: Kynan Ware <47394200+BagToad@users.noreply.github.com> --- pkg/cmd/auth/login/login.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go index 225c6ed06..ca165a22f 100644 --- a/pkg/cmd/auth/login/login.go +++ b/pkg/cmd/auth/login/login.go @@ -68,10 +68,10 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm to writing the token to a plain text file. See %[1]sgh auth status%[1]s for its stored location. - Alternatively, use %[1]s--with-token%[1]s to pass in a token (classic) on standard input. + Alternatively, use %[1]s--with-token%[1]s to pass in a personal access token (classic) on standard input. The minimum required scopes for the token are: %[1]srepo%[1]s, %[1]sread:org%[1]s, and %[1]sgist%[1]s. - Fine-grained tokens are not supported. + Fine-grained personal access tokens are not supported. Alternatively, gh will use the authentication token found in environment variables. This method is most suitable for "headless" use of gh such as in automation. See From 8dbbceaaafbc31d1af9e8c9be1cca58d1e28fa9b Mon Sep 17 00:00:00 2001 From: Aryan Bhosale <36108149+aryanbhosale@users.noreply.github.com> Date: Fri, 3 Jan 2025 10:54:30 +0530 Subject: [PATCH 32/50] print repo url to stdout --- pkg/cmd/repo/fork/fork.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/repo/fork/fork.go b/pkg/cmd/repo/fork/fork.go index 971adcb41..3d62a73cc 100644 --- a/pkg/cmd/repo/fork/fork.go +++ b/pkg/cmd/repo/fork/fork.go @@ -222,7 +222,7 @@ func forkRun(opts *ForkOptions) error { if connectedToTerminal { fmt.Fprintf(stderr, "%s Created fork %s\n", cs.SuccessIconWithColor(cs.Green), cs.Bold(ghrepo.FullName(forkedRepo))) } else { - fmt.Fprintf(stderr, "Created fork %s\n", ghrepo.FullName(forkedRepo)) + fmt.Fprintln(opts.IO.Out, ghrepo.GenerateRepoURL(forkedRepo, "")) } } From 576fa8a3bc97169f24fd5264dc4f4114027308c6 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Thu, 2 Jan 2025 22:29:45 -0800 Subject: [PATCH 33/50] Add test for permissions check for security and analysis edits (#1) --- pkg/cmd/repo/edit/edit.go | 2 +- pkg/cmd/repo/edit/edit_test.go | 29 +++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/repo/edit/edit.go b/pkg/cmd/repo/edit/edit.go index cacc9420d..daa700cfb 100644 --- a/pkg/cmd/repo/edit/edit.go +++ b/pkg/cmd/repo/edit/edit.go @@ -252,7 +252,7 @@ func editRun(ctx context.Context, opts *EditOptions) error { } } - if hasSecurityEdits(opts.Edits) { + if opts.Edits.SecurityAndAnalysis != nil { apiClient := api.NewClientFromHTTP(opts.HTTPClient) repo, err := api.FetchRepository(apiClient, opts.Repository, []string{"viewerCanAdminister"}) if err != nil { diff --git a/pkg/cmd/repo/edit/edit_test.go b/pkg/cmd/repo/edit/edit_test.go index 93b256465..868e300fa 100644 --- a/pkg/cmd/repo/edit/edit_test.go +++ b/pkg/cmd/repo/edit/edit_test.go @@ -220,6 +220,10 @@ func Test_editRun(t *testing.T) { }, }, httpStubs: func(t *testing.T, r *httpmock.Registry) { + r.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(`{"data": { "repository": { "viewerCanAdminister": true } } }`)) + r.Register( httpmock.REST("PATCH", "repos/OWNER/REPO"), httpmock.RESTPayload(200, `{}`, func(payload map[string]interface{}) { @@ -231,6 +235,31 @@ func Test_editRun(t *testing.T) { })) }, }, + { + name: "does not have sufficient permissions for security edits", + opts: EditOptions{ + Repository: ghrepo.NewWithHost("OWNER", "REPO", "github.com"), + Edits: EditRepositoryInput{ + SecurityAndAnalysis: &SecurityAndAnalysisInput{ + EnableAdvancedSecurity: &SecurityAndAnalysisStatus{ + Status: sp("enabled"), + }, + EnableSecretScanning: &SecurityAndAnalysisStatus{ + Status: sp("enabled"), + }, + EnableSecretScanningPushProtection: &SecurityAndAnalysisStatus{ + Status: sp("disabled"), + }, + }, + }, + }, + httpStubs: func(t *testing.T, r *httpmock.Registry) { + r.Register( + httpmock.GraphQL(`query RepositoryInfo\b`), + httpmock.StringResponse(`{"data": { "repository": { "viewerCanAdminister": false } } }`)) + }, + wantsErr: "you do not have sufficient permissions to edit repository security and analysis features", + }, } for _, tt := range tests { From 7a1052ca339a2475fddd08c22cca1eacd8707251 Mon Sep 17 00:00:00 2001 From: Caleb Brose <5447118+cmbrose@users.noreply.github.com> Date: Fri, 3 Jan 2025 20:35:48 +0000 Subject: [PATCH 34/50] Set LocalBranch even if the git config fails --- .../pr-create-without-upstream-config.txtar | 27 +++++++++++++++++++ git/client.go | 3 ++- 2 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 acceptance/testdata/pr/pr-create-without-upstream-config.txtar diff --git a/acceptance/testdata/pr/pr-create-without-upstream-config.txtar b/acceptance/testdata/pr/pr-create-without-upstream-config.txtar new file mode 100644 index 000000000..00f3535a7 --- /dev/null +++ b/acceptance/testdata/pr/pr-create-without-upstream-config.txtar @@ -0,0 +1,27 @@ +# This test is the same as pr-create-basic, except that the git push doesn't include the -u argument +# This causes a git config read to fail during gh pr create, but it should not be fatal + +# Use gh as a credential helper +exec gh auth setup-git + +# Create a repository with a file so it has a default branch +exec gh repo create $ORG/$SCRIPT_NAME-$RANDOM_STRING --add-readme --private + +# Defer repo cleanup +defer gh repo delete --yes $ORG/$SCRIPT_NAME-$RANDOM_STRING + +# Clone the repo +exec gh repo clone $ORG/$SCRIPT_NAME-$RANDOM_STRING + +# Prepare a branch to PR +cd $SCRIPT_NAME-$RANDOM_STRING +exec git checkout -b feature-branch +exec git commit --allow-empty -m 'Empty Commit' +exec git push origin feature-branch + +# Create the PR +exec gh pr create --title 'Feature Title' --body 'Feature Body' + +# Check the PR is indeed created +exec gh pr view +stdout 'Feature Title' diff --git a/git/client.go b/git/client.go index 35b9cf16c..774ce5e76 100644 --- a/git/client.go +++ b/git/client.go @@ -378,6 +378,8 @@ func (c *Client) lookupCommit(ctx context.Context, sha, format string) ([]byte, // ReadBranchConfig parses the `branch.BRANCH.(remote|merge|gh-merge-base)` part of git config. func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (cfg BranchConfig) { + cfg.LocalName = branch + prefix := regexp.QuoteMeta(fmt.Sprintf("branch.%s.", branch)) args := []string{"config", "--get-regexp", fmt.Sprintf("^%s(remote|merge|%s)$", prefix, MergeBaseConfig)} cmd, err := c.Command(ctx, args...) @@ -389,7 +391,6 @@ func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (cfg Branc return } - cfg.LocalName = branch for _, line := range outputLines(out) { parts := strings.SplitN(line, " ", 2) if len(parts) < 2 { From 9d490547b8d20f168e9c665172a3a12204e2f497 Mon Sep 17 00:00:00 2001 From: Caleb Brose <5447118+cmbrose@users.noreply.github.com> Date: Fri, 3 Jan 2025 20:39:12 +0000 Subject: [PATCH 35/50] Alternative: remove LocalBranch from BranchConfig --- git/client.go | 2 -- git/client_test.go | 2 +- git/objects.go | 2 -- pkg/cmd/pr/create/create.go | 6 +++--- 4 files changed, 4 insertions(+), 8 deletions(-) diff --git a/git/client.go b/git/client.go index 774ce5e76..1a6d9ae7f 100644 --- a/git/client.go +++ b/git/client.go @@ -378,8 +378,6 @@ func (c *Client) lookupCommit(ctx context.Context, sha, format string) ([]byte, // ReadBranchConfig parses the `branch.BRANCH.(remote|merge|gh-merge-base)` part of git config. func (c *Client) ReadBranchConfig(ctx context.Context, branch string) (cfg BranchConfig) { - cfg.LocalName = branch - prefix := regexp.QuoteMeta(fmt.Sprintf("branch.%s.", branch)) args := []string{"config", "--get-regexp", fmt.Sprintf("^%s(remote|merge|%s)$", prefix, MergeBaseConfig)} cmd, err := c.Command(ctx, args...) diff --git a/git/client_test.go b/git/client_test.go index 0ec4f7334..fff1397f3 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -737,7 +737,7 @@ func TestClientReadBranchConfig(t *testing.T) { name: "read branch config", cmdStdout: "branch.trunk.remote origin\nbranch.trunk.merge refs/heads/trunk\nbranch.trunk.gh-merge-base trunk", wantCmdArgs: `path/to/git config --get-regexp ^branch\.trunk\.(remote|merge|gh-merge-base)$`, - wantBranchConfig: BranchConfig{LocalName: "trunk", RemoteName: "origin", MergeRef: "refs/heads/trunk", MergeBase: "trunk"}, + wantBranchConfig: BranchConfig{RemoteName: "origin", MergeRef: "refs/heads/trunk", MergeBase: "trunk"}, }, } for _, tt := range tests { diff --git a/git/objects.go b/git/objects.go index f371429dd..ae058a01c 100644 --- a/git/objects.go +++ b/git/objects.go @@ -71,8 +71,6 @@ type Commit struct { } type BranchConfig struct { - // LocalName of the branch. - LocalName string RemoteName string RemoteURL *url.URL // MergeBase is the optional base branch to target in a new PR if `--base` is not specified. diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 3bfa768b7..6a1e13849 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -518,7 +518,7 @@ func initDefaultTitleBody(ctx CreateContext, state *shared.IssueMetadataState, u return nil } -func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, headBranchConfig *git.BranchConfig) *git.TrackingRef { +func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, localBranchName string, headBranchConfig *git.BranchConfig) *git.TrackingRef { refsForLookup := []string{"HEAD"} var trackingRefs []git.TrackingRef @@ -534,7 +534,7 @@ func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, h for _, remote := range remotes { tr := git.TrackingRef{ RemoteName: remote.Name, - BranchName: headBranchConfig.LocalName, + BranchName: localBranchName, } trackingRefs = append(trackingRefs, tr) refsForLookup = append(refsForLookup, tr.String()) @@ -647,7 +647,7 @@ func NewCreateContext(opts *CreateOptions) (*CreateContext, error) { headBranchConfig := gitClient.ReadBranchConfig(context.Background(), headBranch) if isPushEnabled { // determine whether the head branch is already pushed to a remote - if pushedTo := determineTrackingBranch(gitClient, remotes, &headBranchConfig); pushedTo != nil { + if pushedTo := determineTrackingBranch(gitClient, remotes, headBranch, &headBranchConfig); pushedTo != nil { isPushEnabled = false if r, err := remotes.FindByName(pushedTo.RemoteName); err == nil { headRepo = r From 67749480d566fd2cc268ea68155d132197a5e258 Mon Sep 17 00:00:00 2001 From: Caleb Brose <5447118+cmbrose@users.noreply.github.com> Date: Fri, 3 Jan 2025 20:45:20 +0000 Subject: [PATCH 36/50] Fix test --- pkg/cmd/pr/create/create_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 27220d052..9d6584e6c 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -1719,7 +1719,7 @@ func Test_determineTrackingBranch(t *testing.T) { GitPath: "some/path/git", } headBranchConfig := gitClient.ReadBranchConfig(ctx.Background(), "feature") - ref := determineTrackingBranch(gitClient, tt.remotes, &headBranchConfig) + ref := determineTrackingBranch(gitClient, tt.remotes, "feature", &headBranchConfig) tt.assert(ref, t) }) } From efec5d92501b582111254cae679afecb157cc3a2 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Fri, 3 Jan 2025 15:50:54 -0800 Subject: [PATCH 37/50] Fixed test for stdout in non-tty use case of repo fork --- pkg/cmd/repo/fork/fork_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pkg/cmd/repo/fork/fork_test.go b/pkg/cmd/repo/fork/fork_test.go index 0252602fe..1f0b9cef1 100644 --- a/pkg/cmd/repo/fork/fork_test.go +++ b/pkg/cmd/repo/fork/fork_test.go @@ -390,6 +390,7 @@ func TestRepoFork(t *testing.T) { }, }, httpStubs: forkPost, + wantOut: "https://github.com/someone/REPO\n", }, { name: "implicit nontty remote exists", @@ -424,11 +425,13 @@ func TestRepoFork(t *testing.T) { cs.Register("git remote rename origin upstream", 0, "") cs.Register(`git remote add origin https://github.com/someone/REPO.git`, 0, "") }, + wantOut: "https://github.com/someone/REPO\n", }, { name: "implicit nontty no args", opts: &ForkOptions{}, httpStubs: forkPost, + wantOut: "https://github.com/someone/REPO\n", }, { name: "passes git flags", @@ -561,6 +564,7 @@ func TestRepoFork(t *testing.T) { Repository: "OWNER/REPO", }, httpStubs: forkPost, + wantOut: "https://github.com/someone/REPO\n", }, { name: "repo arg nontty repo already exists", @@ -604,6 +608,7 @@ func TestRepoFork(t *testing.T) { cs.Register(`git -C REPO fetch upstream`, 0, "") cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") }, + wantOut: "https://github.com/someone/REPO\n", }, { name: "non tty repo arg with fork-name", @@ -640,6 +645,7 @@ func TestRepoFork(t *testing.T) { httpmock.StringResponse(renameResult)) }, wantErrOut: "", + wantOut: "https://github.com/OWNER/REPO\n", }, { name: "tty repo arg with fork-name", @@ -694,6 +700,7 @@ func TestRepoFork(t *testing.T) { cs.Register(`git -C REPO fetch upstream`, 0, "") cs.Register(`git -C REPO config --add remote.upstream.gh-resolved base`, 0, "") }, + wantOut: "https://github.com/someone/REPO\n", }, { name: "does not retry clone if error occurs and exit code is not 128", From dd32a9cc94f61c823e81568c2822cf4432471a70 Mon Sep 17 00:00:00 2001 From: Josh Soref <2119212+jsoref@users.noreply.github.com> Date: Sun, 5 Jan 2025 23:18:28 -0500 Subject: [PATCH 38/50] Upgrade generated workflows Use `cli/gh-extension-precompile@v2` Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com> --- pkg/cmd/extension/ext_tmpls/goBinWorkflow.yml | 2 +- pkg/cmd/extension/ext_tmpls/otherBinWorkflow.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/extension/ext_tmpls/goBinWorkflow.yml b/pkg/cmd/extension/ext_tmpls/goBinWorkflow.yml index 080019c2a..b804448c6 100644 --- a/pkg/cmd/extension/ext_tmpls/goBinWorkflow.yml +++ b/pkg/cmd/extension/ext_tmpls/goBinWorkflow.yml @@ -13,7 +13,7 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - - uses: cli/gh-extension-precompile@v1 + - uses: cli/gh-extension-precompile@v2 with: generate_attestations: true go_version_file: go.mod diff --git a/pkg/cmd/extension/ext_tmpls/otherBinWorkflow.yml b/pkg/cmd/extension/ext_tmpls/otherBinWorkflow.yml index 78ba05171..4eb99a4a4 100644 --- a/pkg/cmd/extension/ext_tmpls/otherBinWorkflow.yml +++ b/pkg/cmd/extension/ext_tmpls/otherBinWorkflow.yml @@ -11,6 +11,6 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 - - uses: cli/gh-extension-precompile@v1 + - uses: cli/gh-extension-precompile@v2 with: build_script_override: "script/build.sh" From 3695bda6379a1444781110acefe1e0b1306eace3 Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 6 Jan 2025 16:30:48 +0100 Subject: [PATCH 39/50] Clear up --with-token fine grained PAT usage --- pkg/cmd/auth/login/login.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go index ca165a22f..c8011e597 100644 --- a/pkg/cmd/auth/login/login.go +++ b/pkg/cmd/auth/login/login.go @@ -69,9 +69,10 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm stored location. Alternatively, use %[1]s--with-token%[1]s to pass in a personal access token (classic) on standard input. - The minimum required scopes for the token are: %[1]srepo%[1]s, %[1]sread:org%[1]s, and %[1]sgist%[1]s. - - Fine-grained personal access tokens are not supported. + The minimum required scopes for the token are: %[1]srepo%[1]s, %[1]sread:org%[1]s, and %[1]sgist%[1]s. Although + it is possible to pass a fine-grained personal access token to %[1]s--with-token%[1]s, it should be done with + care, as the inherent scoping to certain resources may cause confusing behaviour when interacting with other + resources. Favour setting %[1]sGH_TOKEN$%[1]s for fine-grained personal access token usage. Alternatively, gh will use the authentication token found in environment variables. This method is most suitable for "headless" use of gh such as in automation. See From e7d49fb78c371ba5df66407dbfa9f3aec6e61ea6 Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 6 Jan 2025 17:21:02 +0100 Subject: [PATCH 40/50] Update pkg/cmd/auth/login/login.go Co-authored-by: Andy Feller --- pkg/cmd/auth/login/login.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go index c8011e597..347fcdb6a 100644 --- a/pkg/cmd/auth/login/login.go +++ b/pkg/cmd/auth/login/login.go @@ -69,9 +69,9 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm stored location. Alternatively, use %[1]s--with-token%[1]s to pass in a personal access token (classic) on standard input. - The minimum required scopes for the token are: %[1]srepo%[1]s, %[1]sread:org%[1]s, and %[1]sgist%[1]s. Although - it is possible to pass a fine-grained personal access token to %[1]s--with-token%[1]s, it should be done with - care, as the inherent scoping to certain resources may cause confusing behaviour when interacting with other + The minimum required scopes for the token are: %[1]srepo%[1]s, %[1]sread:org%[1]s, and %[1]sgist%[1]s. + Take care when passing a fine-grained personal access token to %[1]s--with-token%[1]s + as the inherent scoping to certain resources may cause confusing behaviour when interacting with other resources. Favour setting %[1]sGH_TOKEN$%[1]s for fine-grained personal access token usage. Alternatively, gh will use the authentication token found in environment variables. From 3ae4e5da20ebd9f9f46342906c0a930b75310d3b Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 6 Jan 2025 15:14:43 +0100 Subject: [PATCH 41/50] Document and rework pr create tracking branch lookup --- git/objects.go | 12 ++++++++++++ pkg/cmd/pr/create/create.go | 30 +++++++++++++++++++----------- 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/git/objects.go b/git/objects.go index ae058a01c..924a3059e 100644 --- a/git/objects.go +++ b/git/objects.go @@ -1,6 +1,7 @@ package git import ( + "fmt" "net/url" "strings" ) @@ -64,6 +65,17 @@ func (r TrackingRef) String() string { return "refs/remotes/" + r.RemoteName + "/" + r.BranchName } +func ParseTrackingRef(text string) (TrackingRef, error) { + parts := strings.SplitN(string(text), "/", 4) + if len(parts) != 4 { + return TrackingRef{}, fmt.Errorf("invalid tracking ref: %s", text) + } + return TrackingRef{ + RemoteName: parts[2], + BranchName: parts[3], + }, nil +} + type Commit struct { Sha string Title string diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 6a1e13849..df62dc889 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -519,15 +519,14 @@ func initDefaultTitleBody(ctx CreateContext, state *shared.IssueMetadataState, u } func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, localBranchName string, headBranchConfig *git.BranchConfig) *git.TrackingRef { + // To try and determine the tracking ref for a local branch, we first construct a collection of refs + // that might be tracking, given the current branch's config, and the list of known remotes. refsForLookup := []string{"HEAD"} - var trackingRefs []git.TrackingRef - - if headBranchConfig.RemoteName != "" { + if headBranchConfig.RemoteName != "" && headBranchConfig.MergeRef != "" { tr := git.TrackingRef{ RemoteName: headBranchConfig.RemoteName, BranchName: strings.TrimPrefix(headBranchConfig.MergeRef, "refs/heads/"), } - trackingRefs = append(trackingRefs, tr) refsForLookup = append(refsForLookup, tr.String()) } @@ -536,22 +535,31 @@ func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, l RemoteName: remote.Name, BranchName: localBranchName, } - trackingRefs = append(trackingRefs, tr) refsForLookup = append(refsForLookup, tr.String()) } + // Then we ask git for details about these refs, for example, refs/remotes/origin/trunk might return a hash + // for the remote tracking branch, trunk, for the remote, origin. If there is no ref, the git client returns + // no ref information. + // + // We also first check for the HEAD ref, so that we have the hash of the currently checked out commit. resolvedRefs, _ := gitClient.ShowRefs(context.Background(), refsForLookup) + + // If there is more than one resolved ref, that means that at least one ref was found in addition to the HEAD. if len(resolvedRefs) > 1 { + headRef := resolvedRefs[0] for _, r := range resolvedRefs[1:] { - if r.Hash != resolvedRefs[0].Hash { + // If the hash of the remote ref doesn't match the hash of HEAD then the remote branch is not in the same + // state, so it can't be used. + if r.Hash != headRef.Hash { continue } - for _, tr := range trackingRefs { - if tr.String() != r.Name { - continue - } - return &tr + // Otherwise we can parse the returned ref into a tracking ref and return that + trackingRef, err := git.ParseTrackingRef(r.Name) + if err != nil { + return nil } + return &trackingRef } } From dc077dc09ba68843e973b715c448e376135e6c67 Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 6 Jan 2025 15:44:34 +0100 Subject: [PATCH 42/50] Panic if tracking ref can't be reconstructed --- git/objects.go | 12 ------------ pkg/cmd/pr/create/create.go | 16 ++++++++++++---- 2 files changed, 12 insertions(+), 16 deletions(-) diff --git a/git/objects.go b/git/objects.go index 924a3059e..ae058a01c 100644 --- a/git/objects.go +++ b/git/objects.go @@ -1,7 +1,6 @@ package git import ( - "fmt" "net/url" "strings" ) @@ -65,17 +64,6 @@ func (r TrackingRef) String() string { return "refs/remotes/" + r.RemoteName + "/" + r.BranchName } -func ParseTrackingRef(text string) (TrackingRef, error) { - parts := strings.SplitN(string(text), "/", 4) - if len(parts) != 4 { - return TrackingRef{}, fmt.Errorf("invalid tracking ref: %s", text) - } - return TrackingRef{ - RemoteName: parts[2], - BranchName: parts[3], - }, nil -} - type Commit struct { Sha string Title string diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index df62dc889..d7032714a 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -555,10 +555,7 @@ func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, l continue } // Otherwise we can parse the returned ref into a tracking ref and return that - trackingRef, err := git.ParseTrackingRef(r.Name) - if err != nil { - return nil - } + trackingRef := mustParseTrackingRef(r.Name) return &trackingRef } } @@ -566,6 +563,17 @@ func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, l return nil } +func mustParseTrackingRef(text string) git.TrackingRef { + parts := strings.SplitN(string(text), "/", 4) + if len(parts) != 4 { + panic(fmt.Errorf("invalid tracking ref: %s", text)) + } + return git.TrackingRef{ + RemoteName: parts[2], + BranchName: parts[3], + } +} + func NewIssueState(ctx CreateContext, opts CreateOptions) (*shared.IssueMetadataState, error) { var milestoneTitles []string if opts.Milestone != "" { From 05764b8114e59411cf41723124d83fbb400d9395 Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 6 Jan 2025 15:47:10 +0100 Subject: [PATCH 43/50] Don't use pointer for determineTrackingBranch branchConfig --- pkg/cmd/pr/create/create.go | 4 ++-- pkg/cmd/pr/create/create_test.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index d7032714a..40913db64 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -518,7 +518,7 @@ func initDefaultTitleBody(ctx CreateContext, state *shared.IssueMetadataState, u return nil } -func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, localBranchName string, headBranchConfig *git.BranchConfig) *git.TrackingRef { +func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, localBranchName string, headBranchConfig git.BranchConfig) *git.TrackingRef { // To try and determine the tracking ref for a local branch, we first construct a collection of refs // that might be tracking, given the current branch's config, and the list of known remotes. refsForLookup := []string{"HEAD"} @@ -663,7 +663,7 @@ func NewCreateContext(opts *CreateOptions) (*CreateContext, error) { headBranchConfig := gitClient.ReadBranchConfig(context.Background(), headBranch) if isPushEnabled { // determine whether the head branch is already pushed to a remote - if pushedTo := determineTrackingBranch(gitClient, remotes, headBranch, &headBranchConfig); pushedTo != nil { + if pushedTo := determineTrackingBranch(gitClient, remotes, headBranch, headBranchConfig); pushedTo != nil { isPushEnabled = false if r, err := remotes.FindByName(pushedTo.RemoteName); err == nil { headRepo = r diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 9d6584e6c..8d5a1d1b3 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -1719,7 +1719,7 @@ func Test_determineTrackingBranch(t *testing.T) { GitPath: "some/path/git", } headBranchConfig := gitClient.ReadBranchConfig(ctx.Background(), "feature") - ref := determineTrackingBranch(gitClient, tt.remotes, "feature", &headBranchConfig) + ref := determineTrackingBranch(gitClient, tt.remotes, "feature", headBranchConfig) tt.assert(ref, t) }) } From 27bd4b2aec09f1242177821ad72a01b01163c998 Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 6 Jan 2025 15:49:53 +0100 Subject: [PATCH 44/50] Doc determineTrackingBranch --- pkg/cmd/pr/create/create.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 40913db64..b05c971fe 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -518,6 +518,8 @@ func initDefaultTitleBody(ctx CreateContext, state *shared.IssueMetadataState, u return nil } +// determineTrackingBranch is intended to try and find a remote branch on the same commit as the currently checked out +// HEAD, i.e. the local branch. func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, localBranchName string, headBranchConfig git.BranchConfig) *git.TrackingRef { // To try and determine the tracking ref for a local branch, we first construct a collection of refs // that might be tracking, given the current branch's config, and the list of known remotes. From b8c167970b70205cba2fd22e2bb2da378229f0d7 Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 6 Jan 2025 15:55:35 +0100 Subject: [PATCH 45/50] Avoid pointer return from determineTrackingBranch --- pkg/cmd/pr/create/create.go | 17 ++++++++-------- pkg/cmd/pr/create/create_test.go | 33 +++++++++++++++++++------------- 2 files changed, 28 insertions(+), 22 deletions(-) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index b05c971fe..00993e0fa 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -518,9 +518,9 @@ func initDefaultTitleBody(ctx CreateContext, state *shared.IssueMetadataState, u return nil } -// determineTrackingBranch is intended to try and find a remote branch on the same commit as the currently checked out +// tryDetermineTrackingRef is intended to try and find a remote branch on the same commit as the currently checked out // HEAD, i.e. the local branch. -func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, localBranchName string, headBranchConfig git.BranchConfig) *git.TrackingRef { +func tryDetermineTrackingRef(gitClient *git.Client, remotes ghContext.Remotes, localBranchName string, headBranchConfig git.BranchConfig) (git.TrackingRef, bool) { // To try and determine the tracking ref for a local branch, we first construct a collection of refs // that might be tracking, given the current branch's config, and the list of known remotes. refsForLookup := []string{"HEAD"} @@ -557,12 +557,11 @@ func determineTrackingBranch(gitClient *git.Client, remotes ghContext.Remotes, l continue } // Otherwise we can parse the returned ref into a tracking ref and return that - trackingRef := mustParseTrackingRef(r.Name) - return &trackingRef + return mustParseTrackingRef(r.Name), true } } - return nil + return git.TrackingRef{}, false } func mustParseTrackingRef(text string) git.TrackingRef { @@ -665,14 +664,14 @@ func NewCreateContext(opts *CreateOptions) (*CreateContext, error) { headBranchConfig := gitClient.ReadBranchConfig(context.Background(), headBranch) if isPushEnabled { // determine whether the head branch is already pushed to a remote - if pushedTo := determineTrackingBranch(gitClient, remotes, headBranch, headBranchConfig); pushedTo != nil { + if trackingRef, found := tryDetermineTrackingRef(gitClient, remotes, headBranch, headBranchConfig); found { isPushEnabled = false - if r, err := remotes.FindByName(pushedTo.RemoteName); err == nil { + if r, err := remotes.FindByName(trackingRef.RemoteName); err == nil { headRepo = r headRemote = r - headBranchLabel = pushedTo.BranchName + headBranchLabel = trackingRef.BranchName if !ghrepo.IsSame(baseRepo, headRepo) { - headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), pushedTo.BranchName) + headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), trackingRef.BranchName) } } } diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 8d5a1d1b3..acb35f557 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -1622,12 +1622,12 @@ func Test_createRun(t *testing.T) { } } -func Test_determineTrackingBranch(t *testing.T) { +func Test_tryDetermineTrackingRef(t *testing.T) { tests := []struct { name string cmdStubs func(*run.CommandStubber) remotes context.Remotes - assert func(ref *git.TrackingRef, t *testing.T) + assert func(ref git.TrackingRef, found bool, t *testing.T) }{ { name: "empty", @@ -1635,8 +1635,9 @@ func Test_determineTrackingBranch(t *testing.T) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD`, 0, "abc HEAD") }, - assert: func(ref *git.TrackingRef, t *testing.T) { - assert.Nil(t, ref) + assert: func(ref git.TrackingRef, found bool, t *testing.T) { + assert.Zero(t, ref) + assert.False(t, found) }, }, { @@ -1655,8 +1656,9 @@ func Test_determineTrackingBranch(t *testing.T) { Repo: ghrepo.New("octocat", "Spoon-Knife"), }, }, - assert: func(ref *git.TrackingRef, t *testing.T) { - assert.Nil(t, ref) + assert: func(ref git.TrackingRef, found bool, t *testing.T) { + assert.Zero(t, ref) + assert.False(t, found) }, }, { @@ -1679,9 +1681,12 @@ func Test_determineTrackingBranch(t *testing.T) { Repo: ghrepo.New("octocat", "Spoon-Knife"), }, }, - assert: func(ref *git.TrackingRef, t *testing.T) { - assert.Equal(t, "upstream", ref.RemoteName) - assert.Equal(t, "feature", ref.BranchName) + assert: func(ref git.TrackingRef, found bool, t *testing.T) { + assert.Equal(t, git.TrackingRef{ + RemoteName: "upstream", + BranchName: "feature", + }, ref) + assert.True(t, found) }, }, { @@ -1702,8 +1707,9 @@ func Test_determineTrackingBranch(t *testing.T) { Repo: ghrepo.New("hubot", "Spoon-Knife"), }, }, - assert: func(ref *git.TrackingRef, t *testing.T) { - assert.Nil(t, ref) + assert: func(ref git.TrackingRef, found bool, t *testing.T) { + assert.Zero(t, ref) + assert.False(t, found) }, }, } @@ -1719,8 +1725,9 @@ func Test_determineTrackingBranch(t *testing.T) { GitPath: "some/path/git", } headBranchConfig := gitClient.ReadBranchConfig(ctx.Background(), "feature") - ref := determineTrackingBranch(gitClient, tt.remotes, "feature", headBranchConfig) - tt.assert(ref, t) + ref, found := tryDetermineTrackingRef(gitClient, tt.remotes, "feature", headBranchConfig) + + tt.assert(ref, found, t) }) } } From 57ba5e56082e442e3e4e856ba8ca4aec703c8499 Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 6 Jan 2025 15:57:14 +0100 Subject: [PATCH 46/50] Rework tryDetermineTrackingRef tests --- pkg/cmd/pr/create/create_test.go | 40 ++++++++++++++------------------ 1 file changed, 17 insertions(+), 23 deletions(-) diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index acb35f557..4785a9d99 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -1624,10 +1624,11 @@ func Test_createRun(t *testing.T) { func Test_tryDetermineTrackingRef(t *testing.T) { tests := []struct { - name string - cmdStubs func(*run.CommandStubber) - remotes context.Remotes - assert func(ref git.TrackingRef, found bool, t *testing.T) + name string + cmdStubs func(*run.CommandStubber) + remotes context.Remotes + expectedTrackingRef git.TrackingRef + expectedFound bool }{ { name: "empty", @@ -1635,10 +1636,8 @@ func Test_tryDetermineTrackingRef(t *testing.T) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD`, 0, "abc HEAD") }, - assert: func(ref git.TrackingRef, found bool, t *testing.T) { - assert.Zero(t, ref) - assert.False(t, found) - }, + expectedTrackingRef: git.TrackingRef{}, + expectedFound: false, }, { name: "no match", @@ -1656,10 +1655,8 @@ func Test_tryDetermineTrackingRef(t *testing.T) { Repo: ghrepo.New("octocat", "Spoon-Knife"), }, }, - assert: func(ref git.TrackingRef, found bool, t *testing.T) { - assert.Zero(t, ref) - assert.False(t, found) - }, + expectedTrackingRef: git.TrackingRef{}, + expectedFound: false, }, { name: "match", @@ -1681,13 +1678,11 @@ func Test_tryDetermineTrackingRef(t *testing.T) { Repo: ghrepo.New("octocat", "Spoon-Knife"), }, }, - assert: func(ref git.TrackingRef, found bool, t *testing.T) { - assert.Equal(t, git.TrackingRef{ - RemoteName: "upstream", - BranchName: "feature", - }, ref) - assert.True(t, found) + expectedTrackingRef: git.TrackingRef{ + RemoteName: "upstream", + BranchName: "feature", }, + expectedFound: true, }, { name: "respect tracking config", @@ -1707,10 +1702,8 @@ func Test_tryDetermineTrackingRef(t *testing.T) { Repo: ghrepo.New("hubot", "Spoon-Knife"), }, }, - assert: func(ref git.TrackingRef, found bool, t *testing.T) { - assert.Zero(t, ref) - assert.False(t, found) - }, + expectedTrackingRef: git.TrackingRef{}, + expectedFound: false, }, } for _, tt := range tests { @@ -1727,7 +1720,8 @@ func Test_tryDetermineTrackingRef(t *testing.T) { headBranchConfig := gitClient.ReadBranchConfig(ctx.Background(), "feature") ref, found := tryDetermineTrackingRef(gitClient, tt.remotes, "feature", headBranchConfig) - tt.assert(ref, found, t) + assert.Equal(t, tt.expectedTrackingRef, ref) + assert.Equal(t, tt.expectedFound, found) }) } } From 62ecb1c84dbb37ea908fa25c7f5e11a27faaf51c Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 6 Jan 2025 16:10:12 +0100 Subject: [PATCH 47/50] Make tryDetermineTrackingRef tests more respective of reality Though it doesn't really matter, in practice upstream is always going to come before origin. --- pkg/cmd/pr/create/create.go | 4 +++- pkg/cmd/pr/create/create_test.go | 26 +++++++++++++------------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 00993e0fa..372daf86c 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -519,7 +519,9 @@ func initDefaultTitleBody(ctx CreateContext, state *shared.IssueMetadataState, u } // tryDetermineTrackingRef is intended to try and find a remote branch on the same commit as the currently checked out -// HEAD, i.e. the local branch. +// HEAD, i.e. the local branch. If there are multiple branches that might match, the first remote is chosen, which in +// practice is determined by the sorting algorithm applied much earlier in the process, roughly "upstream", "github", "origin", +// and then everything else unstably sorted. func tryDetermineTrackingRef(gitClient *git.Client, remotes ghContext.Remotes, localBranchName string, headBranchConfig git.BranchConfig) (git.TrackingRef, bool) { // To try and determine the tracking ref for a local branch, we first construct a collection of refs // that might be tracking, given the current branch's config, and the list of known remotes. diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 4785a9d99..c5127bcc6 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -1643,17 +1643,17 @@ func Test_tryDetermineTrackingRef(t *testing.T) { name: "no match", cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") - cs.Register("git show-ref --verify -- HEAD refs/remotes/origin/feature refs/remotes/upstream/feature", 0, "abc HEAD\nbca refs/remotes/origin/feature") + cs.Register("git show-ref --verify -- HEAD refs/remotes/upstream/feature refs/remotes/origin/feature", 0, "abc HEAD\nbca refs/remotes/upstream/feature") }, remotes: context.Remotes{ - &context.Remote{ - Remote: &git.Remote{Name: "origin"}, - Repo: ghrepo.New("hubot", "Spoon-Knife"), - }, &context.Remote{ Remote: &git.Remote{Name: "upstream"}, Repo: ghrepo.New("octocat", "Spoon-Knife"), }, + &context.Remote{ + Remote: &git.Remote{Name: "origin"}, + Repo: ghrepo.New("hubot", "Spoon-Knife"), + }, }, expectedTrackingRef: git.TrackingRef{}, expectedFound: false, @@ -1662,24 +1662,24 @@ func Test_tryDetermineTrackingRef(t *testing.T) { name: "match", cmdStubs: func(cs *run.CommandStubber) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") - cs.Register(`git show-ref --verify -- HEAD refs/remotes/origin/feature refs/remotes/upstream/feature$`, 0, heredoc.Doc(` + cs.Register(`git show-ref --verify -- HEAD refs/remotes/upstream/feature refs/remotes/origin/feature$`, 0, heredoc.Doc(` deadbeef HEAD - deadb00f refs/remotes/origin/feature - deadbeef refs/remotes/upstream/feature + deadb00f refs/remotes/upstream/feature + deadbeef refs/remotes/origin/feature `)) }, remotes: context.Remotes{ - &context.Remote{ - Remote: &git.Remote{Name: "origin"}, - Repo: ghrepo.New("hubot", "Spoon-Knife"), - }, &context.Remote{ Remote: &git.Remote{Name: "upstream"}, Repo: ghrepo.New("octocat", "Spoon-Knife"), }, + &context.Remote{ + Remote: &git.Remote{Name: "origin"}, + Repo: ghrepo.New("hubot", "Spoon-Knife"), + }, }, expectedTrackingRef: git.TrackingRef{ - RemoteName: "upstream", + RemoteName: "origin", BranchName: "feature", }, expectedFound: true, From 8b5073d6172f9cfe4ad4c8ba157ee35bff3f6ed6 Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 6 Jan 2025 16:58:35 +0100 Subject: [PATCH 48/50] Move trackingRef into pr create package --- git/objects.go | 10 ------ pkg/cmd/pr/create/create.go | 54 +++++++++++++++++++------------- pkg/cmd/pr/create/create_test.go | 14 ++++----- 3 files changed, 39 insertions(+), 39 deletions(-) diff --git a/git/objects.go b/git/objects.go index ae058a01c..c09683042 100644 --- a/git/objects.go +++ b/git/objects.go @@ -54,16 +54,6 @@ type Ref struct { Name string } -// TrackingRef represents a ref for a remote tracking branch. -type TrackingRef struct { - RemoteName string - BranchName string -} - -func (r TrackingRef) String() string { - return "refs/remotes/" + r.RemoteName + "/" + r.BranchName -} - type Commit struct { Sha string Title string diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 372daf86c..46a4e284e 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -518,26 +518,47 @@ func initDefaultTitleBody(ctx CreateContext, state *shared.IssueMetadataState, u return nil } +// trackingRef represents a ref for a remote tracking branch. +type trackingRef struct { + remoteName string + branchName string +} + +func (r trackingRef) String() string { + return "refs/remotes/" + r.remoteName + "/" + r.branchName +} + +func mustParseTrackingRef(text string) trackingRef { + parts := strings.SplitN(string(text), "/", 4) + if len(parts) != 4 { + panic(fmt.Errorf("invalid tracking ref: %s", text)) + } + return trackingRef{ + remoteName: parts[2], + branchName: parts[3], + } +} + // tryDetermineTrackingRef is intended to try and find a remote branch on the same commit as the currently checked out // HEAD, i.e. the local branch. If there are multiple branches that might match, the first remote is chosen, which in // practice is determined by the sorting algorithm applied much earlier in the process, roughly "upstream", "github", "origin", // and then everything else unstably sorted. -func tryDetermineTrackingRef(gitClient *git.Client, remotes ghContext.Remotes, localBranchName string, headBranchConfig git.BranchConfig) (git.TrackingRef, bool) { +func tryDetermineTrackingRef(gitClient *git.Client, remotes ghContext.Remotes, localBranchName string, headBranchConfig git.BranchConfig) (trackingRef, bool) { // To try and determine the tracking ref for a local branch, we first construct a collection of refs // that might be tracking, given the current branch's config, and the list of known remotes. refsForLookup := []string{"HEAD"} if headBranchConfig.RemoteName != "" && headBranchConfig.MergeRef != "" { - tr := git.TrackingRef{ - RemoteName: headBranchConfig.RemoteName, - BranchName: strings.TrimPrefix(headBranchConfig.MergeRef, "refs/heads/"), + tr := trackingRef{ + remoteName: headBranchConfig.RemoteName, + branchName: strings.TrimPrefix(headBranchConfig.MergeRef, "refs/heads/"), } refsForLookup = append(refsForLookup, tr.String()) } for _, remote := range remotes { - tr := git.TrackingRef{ - RemoteName: remote.Name, - BranchName: localBranchName, + tr := trackingRef{ + remoteName: remote.Name, + branchName: localBranchName, } refsForLookup = append(refsForLookup, tr.String()) } @@ -563,18 +584,7 @@ func tryDetermineTrackingRef(gitClient *git.Client, remotes ghContext.Remotes, l } } - return git.TrackingRef{}, false -} - -func mustParseTrackingRef(text string) git.TrackingRef { - parts := strings.SplitN(string(text), "/", 4) - if len(parts) != 4 { - panic(fmt.Errorf("invalid tracking ref: %s", text)) - } - return git.TrackingRef{ - RemoteName: parts[2], - BranchName: parts[3], - } + return trackingRef{}, false } func NewIssueState(ctx CreateContext, opts CreateOptions) (*shared.IssueMetadataState, error) { @@ -668,12 +678,12 @@ func NewCreateContext(opts *CreateOptions) (*CreateContext, error) { // determine whether the head branch is already pushed to a remote if trackingRef, found := tryDetermineTrackingRef(gitClient, remotes, headBranch, headBranchConfig); found { isPushEnabled = false - if r, err := remotes.FindByName(trackingRef.RemoteName); err == nil { + if r, err := remotes.FindByName(trackingRef.remoteName); err == nil { headRepo = r headRemote = r - headBranchLabel = trackingRef.BranchName + headBranchLabel = trackingRef.branchName if !ghrepo.IsSame(baseRepo, headRepo) { - headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), trackingRef.BranchName) + headBranchLabel = fmt.Sprintf("%s:%s", headRepo.RepoOwner(), trackingRef.branchName) } } } diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index c5127bcc6..6c0ff11e2 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -1627,7 +1627,7 @@ func Test_tryDetermineTrackingRef(t *testing.T) { name string cmdStubs func(*run.CommandStubber) remotes context.Remotes - expectedTrackingRef git.TrackingRef + expectedTrackingRef trackingRef expectedFound bool }{ { @@ -1636,7 +1636,7 @@ func Test_tryDetermineTrackingRef(t *testing.T) { cs.Register(`git config --get-regexp.+branch\\\.feature\\\.`, 0, "") cs.Register(`git show-ref --verify -- HEAD`, 0, "abc HEAD") }, - expectedTrackingRef: git.TrackingRef{}, + expectedTrackingRef: trackingRef{}, expectedFound: false, }, { @@ -1655,7 +1655,7 @@ func Test_tryDetermineTrackingRef(t *testing.T) { Repo: ghrepo.New("hubot", "Spoon-Knife"), }, }, - expectedTrackingRef: git.TrackingRef{}, + expectedTrackingRef: trackingRef{}, expectedFound: false, }, { @@ -1678,9 +1678,9 @@ func Test_tryDetermineTrackingRef(t *testing.T) { Repo: ghrepo.New("hubot", "Spoon-Knife"), }, }, - expectedTrackingRef: git.TrackingRef{ - RemoteName: "origin", - BranchName: "feature", + expectedTrackingRef: trackingRef{ + remoteName: "origin", + branchName: "feature", }, expectedFound: true, }, @@ -1702,7 +1702,7 @@ func Test_tryDetermineTrackingRef(t *testing.T) { Repo: ghrepo.New("hubot", "Spoon-Knife"), }, }, - expectedTrackingRef: git.TrackingRef{}, + expectedTrackingRef: trackingRef{}, expectedFound: false, }, } From c3b41e87b89aad1f8ea3fcbe4c93c08c3a50b3f9 Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 6 Jan 2025 17:00:16 +0100 Subject: [PATCH 49/50] Panic mustParseTrackingRef if format is incorrect --- pkg/cmd/pr/create/create.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 46a4e284e..db160b419 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -530,9 +530,16 @@ func (r trackingRef) String() string { func mustParseTrackingRef(text string) trackingRef { parts := strings.SplitN(string(text), "/", 4) + // The only place this is called is tryDetermineTrackingRef, where we are reconstructing + // the same tracking ref we passed in. If it doesn't match the expected format, this is a + // programmer error we want to know about, so it's ok to panic. if len(parts) != 4 { - panic(fmt.Errorf("invalid tracking ref: %s", text)) + panic(fmt.Errorf("tracking ref should have four parts: %s", text)) } + if parts[0] != "refs" || parts[1] != "remotes" { + panic(fmt.Errorf("tracking ref should start with refs/remotes/: %s", text)) + } + return trackingRef{ remoteName: parts[2], branchName: parts[3], From 757966ca7cdd222b24c04eca2db8321db61a464f Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Mon, 6 Jan 2025 14:27:03 -0500 Subject: [PATCH 50/50] Bump cli/go-gh for indirect security vulnerability --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index 98ee0280f..f2f1ce812 100644 --- a/go.mod +++ b/go.mod @@ -11,7 +11,7 @@ require ( github.com/cenkalti/backoff/v4 v4.3.0 github.com/charmbracelet/glamour v0.7.0 github.com/charmbracelet/lipgloss v0.10.1-0.20240413172830-d0be07ea6b9c - github.com/cli/go-gh/v2 v2.11.1 + github.com/cli/go-gh/v2 v2.11.2 github.com/cli/go-internal v0.0.0-20241025142207-6c48bcd5ce24 github.com/cli/oauth v1.1.1 github.com/cli/safeexec v1.0.1 diff --git a/go.sum b/go.sum index 49e104d07..1b48f0c27 100644 --- a/go.sum +++ b/go.sum @@ -95,8 +95,8 @@ github.com/charmbracelet/x/exp/term v0.0.0-20240425164147-ba2a9512b05f/go.mod h1 github.com/cli/browser v1.0.0/go.mod h1:IEWkHYbLjkhtjwwWlwTHW2lGxeS5gezEQBMLTwDHf5Q= github.com/cli/browser v1.3.0 h1:LejqCrpWr+1pRqmEPDGnTZOjsMe7sehifLynZJuqJpo= github.com/cli/browser v1.3.0/go.mod h1:HH8s+fOAxjhQoBUAsKuPCbqUuxZDhQ2/aD+SzsEfBTk= -github.com/cli/go-gh/v2 v2.11.1 h1:amAyfqMWQTBdue8iTmDUegGZK7c8kk6WCxD9l/wLtGI= -github.com/cli/go-gh/v2 v2.11.1/go.mod h1:MeRoKzXff3ygHu7zP+NVTT+imcHW6p3tpuxHAzRM2xE= +github.com/cli/go-gh/v2 v2.11.2 h1:oad1+sESTPNTiTvh3I3t8UmxuovNDxhwLzeMHk45Q9w= +github.com/cli/go-gh/v2 v2.11.2/go.mod h1:vVFhi3TfjseIW26ED9itAR8gQK0aVThTm8sYrsZ5QTI= github.com/cli/go-internal v0.0.0-20241025142207-6c48bcd5ce24 h1:QDrhR4JA2n3ij9YQN0u5ZeuvRIIvsUGmf5yPlTS0w8E= github.com/cli/go-internal v0.0.0-20241025142207-6c48bcd5ce24/go.mod h1:rr9GNING0onuVw8MnracQHn7PcchnFlP882Y0II2KZk= github.com/cli/oauth v1.1.1 h1:459gD3hSjlKX9B1uXBuiAMdpXBUQ9QGf/NDcCpoQxPs=