From a542011cdce8fb579089a0e6a8284526a19d21d3 Mon Sep 17 00:00:00 2001 From: ChandranshuRao14 Date: Tue, 24 Dec 2024 12:59:42 -0600 Subject: [PATCH 01/24] 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 29371e949126db53cc8d93132d23812a06fc72fd Mon Sep 17 00:00:00 2001 From: ChandranshuRao14 Date: Thu, 26 Dec 2024 22:52:32 -0500 Subject: [PATCH 02/24] 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 23c16c9c4cb033840065e0c1d08dbdf6cce7ca3f Mon Sep 17 00:00:00 2001 From: Michael Hoffman Date: Fri, 20 Dec 2024 13:06:47 -0500 Subject: [PATCH 03/24] 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 04/24] 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 05/24] 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 06/24] 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 07/24] 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 08/24] 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 09/24] 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 10/24] 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 11/24] 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 12/24] 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 13/24] 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 14/24] 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 15/24] 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 16/24] 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 17/24] 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 9558d5b60b190fabb08521518b71bb0a954f0a26 Mon Sep 17 00:00:00 2001 From: nobe4 Date: Thu, 2 Jan 2025 16:53:44 +0100 Subject: [PATCH 18/24] 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 19/24] 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 20/24] 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 21/24] 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 22/24] 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 23/24] 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 576fa8a3bc97169f24fd5264dc4f4114027308c6 Mon Sep 17 00:00:00 2001 From: Tyler McGoffin Date: Thu, 2 Jan 2025 22:29:45 -0800 Subject: [PATCH 24/24] 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 {