From 33975a95e4fde32d066634208e23a66d84f38a30 Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Mon, 6 Jun 2022 12:35:13 -0500 Subject: [PATCH 1/5] Allow repo list to work with GHES earlier than 3.3 --- .../featuredetection/feature_detection.go | 5 ++ .../feature_detection_test.go | 18 ++++++ pkg/cmd/repo/list/list.go | 60 +++++++++++++------ pkg/cmd/repo/list/list_test.go | 43 +++++++++++++ 4 files changed, 108 insertions(+), 18 deletions(-) diff --git a/internal/featuredetection/feature_detection.go b/internal/featuredetection/feature_detection.go index ee96656f1..15eec5852 100644 --- a/internal/featuredetection/feature_detection.go +++ b/internal/featuredetection/feature_detection.go @@ -36,12 +36,14 @@ type RepositoryFeatures struct { IssueTemplateMutation bool IssueTemplateQuery bool PullRequestTemplateQuery bool + VisibilityField bool } var allRepositoryFeatures = RepositoryFeatures{ IssueTemplateMutation: true, IssueTemplateQuery: true, PullRequestTemplateQuery: true, + VisibilityField: true, } type detector struct { @@ -102,6 +104,9 @@ func (d *detector) RepositoryFeatures() (RepositoryFeatures, error) { if field.Name == "pullRequestTemplates" { features.PullRequestTemplateQuery = true } + if field.Name == "visibility" { + features.VisibilityField = true + } } return features, nil diff --git a/internal/featuredetection/feature_detection_test.go b/internal/featuredetection/feature_detection_test.go index 1b6c26273..97ad0c0b8 100644 --- a/internal/featuredetection/feature_detection_test.go +++ b/internal/featuredetection/feature_detection_test.go @@ -72,6 +72,7 @@ func TestRepositoryFeatures(t *testing.T) { IssueTemplateMutation: true, IssueTemplateQuery: true, PullRequestTemplateQuery: true, + VisibilityField: true, }, wantErr: false, }, @@ -105,6 +106,23 @@ func TestRepositoryFeatures(t *testing.T) { }, wantErr: false, }, + { + name: "GHE has visibility field", + hostname: "git.my.org", + queryResponse: map[string]string{ + `query Repository_fields\b`: heredoc.Doc(` + { "data": { "Repository": { "fields": [ + {"name": "visibility"} + ] } } } + `), + }, + wantFeatures: RepositoryFeatures{ + IssueTemplateMutation: true, + IssueTemplateQuery: true, + VisibilityField: true, + }, + wantErr: false, + }, } for _, tt := range tests { diff --git a/pkg/cmd/repo/list/list.go b/pkg/cmd/repo/list/list.go index 321d753e8..d8ca2424b 100644 --- a/pkg/cmd/repo/list/list.go +++ b/pkg/cmd/repo/list/list.go @@ -10,6 +10,7 @@ import ( "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/config" + fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/pkg/text" @@ -21,6 +22,7 @@ type ListOptions struct { Config func() (config.Config, error) IO *iostreams.IOStreams Exporter cmdutil.Exporter + Detector fd.Detector Limit int Owner string @@ -104,7 +106,7 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman return cmd } -var defaultFields = []string{"nameWithOwner", "description", "isPrivate", "isFork", "isArchived", "createdAt", "pushedAt", "visibility"} +var defaultFields = []string{"nameWithOwner", "description", "isPrivate", "isFork", "isArchived", "createdAt", "pushedAt"} func listRun(opts *ListOptions) error { httpClient, err := opts.HttpClient() @@ -112,20 +114,6 @@ func listRun(opts *ListOptions) error { return err } - filter := FilterOptions{ - Visibility: opts.Visibility, - Fork: opts.Fork, - Source: opts.Source, - Language: opts.Language, - Topic: opts.Topic, - Archived: opts.Archived, - NonArchived: opts.NonArchived, - Fields: defaultFields, - } - if opts.Exporter != nil { - filter.Fields = opts.Exporter.Fields() - } - cfg, err := opts.Config() if err != nil { return err @@ -136,6 +124,33 @@ func listRun(opts *ListOptions) error { return err } + if opts.Detector == nil { + opts.Detector = fd.NewDetector(httpClient, host) + } + features, err := opts.Detector.RepositoryFeatures() + if err != nil { + return err + } + + fields := defaultFields + if features.VisibilityField { + fields = append(defaultFields, "visibility") + } + + filter := FilterOptions{ + Visibility: opts.Visibility, + Fork: opts.Fork, + Source: opts.Source, + Language: opts.Language, + Topic: opts.Topic, + Archived: opts.Archived, + NonArchived: opts.NonArchived, + Fields: fields, + } + if opts.Exporter != nil { + filter.Fields = opts.Exporter.Fields() + } + listResult, err := listRepos(httpClient, host, opts.Limit, opts.Owner, filter) if err != nil { return err @@ -158,7 +173,9 @@ func listRun(opts *ListOptions) error { info := repoInfo(repo) infoColor := cs.Gray - if repo.Visibility != "PUBLIC" { + if (repo.Visibility != "" && + repo.Visibility != "PUBLIC") || + repo.IsPrivate { infoColor = cs.Yellow } @@ -208,8 +225,15 @@ func listHeader(owner string, matchCount, totalMatchCount int, hasFilters bool) } func repoInfo(r api.Repository) string { - tags := []string{ - strings.ToLower(r.Visibility), + tags := []string{} + if r.Visibility != "" { + tags = append(tags, strings.ToLower(r.Visibility)) + } else { + if r.IsPrivate { + tags = append(tags, "private") + } else { + tags = append(tags, "public") + } } if r.IsFork { diff --git a/pkg/cmd/repo/list/list_test.go b/pkg/cmd/repo/list/list_test.go index 262ff6ffb..8d0bf72b2 100644 --- a/pkg/cmd/repo/list/list_test.go +++ b/pkg/cmd/repo/list/list_test.go @@ -4,6 +4,7 @@ import ( "bytes" "io" "net/http" + "strings" "testing" "time" @@ -13,6 +14,7 @@ import ( "github.com/stretchr/testify/require" "github.com/cli/cli/v2/internal/config" + fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -394,3 +396,44 @@ func TestRepoList_filtering(t *testing.T) { assert.Equal(t, "", output.Stderr()) assert.Equal(t, "\nNo results match your search\n\n", output.String()) } + +func TestRepoList_noVisibilityField(t *testing.T) { + ios, _, stdout, stderr := iostreams.Test() + ios.SetStdoutTTY(false) + ios.SetStdinTTY(false) + ios.SetStderrTTY(false) + + reg := &httpmock.Registry{} + defer reg.Verify(t) + + reg.Register( + httpmock.GraphQL(`query RepositoryList\b`), + httpmock.GraphQLQuery(`{"data":{"repositoryOwner":{"login":"octocat","repositories":{"totalCount":0}}}}`, + func(query string, params map[string]interface{}) { + assert.False(t, strings.Contains(query, "visibility")) + }, + ), + ) + + opts := ListOptions{ + IO: ios, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + Config: func() (config.Config, error) { + return config.InheritEnv(config.NewBlankConfig()), nil + }, + Now: func() time.Time { + t, _ := time.Parse(time.RFC822, "19 Feb 21 15:00 UTC") + return t + }, + Limit: 30, + Detector: &fd.DisabledDetectorMock{}, + } + + err := listRun(&opts) + + assert.NoError(t, err) + assert.Equal(t, "", stderr.String()) + assert.Equal(t, "", stdout.String()) +} From 89886eeaf8908037265a447e6ee9f247a56f5082 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 7 Jun 2022 17:09:17 +0200 Subject: [PATCH 2/5] Fix repo visibility label in `search repos` output The `Visibility` field will be empty for `search` results when made against GHES versions < 3.3. If that is the case, fall back to constructing the label using `IsPrivate` field. --- pkg/cmd/repo/list/list.go | 20 ++++++++++---------- pkg/cmd/search/repos/repos.go | 11 ++++++++++- 2 files changed, 20 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/repo/list/list.go b/pkg/cmd/repo/list/list.go index d8ca2424b..bfd14d45b 100644 --- a/pkg/cmd/repo/list/list.go +++ b/pkg/cmd/repo/list/list.go @@ -225,16 +225,7 @@ func listHeader(owner string, matchCount, totalMatchCount int, hasFilters bool) } func repoInfo(r api.Repository) string { - tags := []string{} - if r.Visibility != "" { - tags = append(tags, strings.ToLower(r.Visibility)) - } else { - if r.IsPrivate { - tags = append(tags, "private") - } else { - tags = append(tags, "public") - } - } + tags := []string{visibilityLabel(r)} if r.IsFork { tags = append(tags, "fork") @@ -245,3 +236,12 @@ func repoInfo(r api.Repository) string { return strings.Join(tags, ", ") } + +func visibilityLabel(repo api.Repository) string { + if repo.Visibility != "" { + return strings.ToLower(repo.Visibility) + } else if repo.IsPrivate { + return "private" + } + return "public" +} diff --git a/pkg/cmd/search/repos/repos.go b/pkg/cmd/search/repos/repos.go index 3c60ef1eb..57f376672 100644 --- a/pkg/cmd/search/repos/repos.go +++ b/pkg/cmd/search/repos/repos.go @@ -154,7 +154,7 @@ func displayResults(io *iostreams.IOStreams, results search.RepositoriesResult) cs := io.ColorScheme() tp := utils.NewTablePrinter(io) for _, repo := range results.Items { - tags := []string{repo.Visibility} + tags := []string{visibilityLabel(repo)} if repo.IsFork { tags = append(tags, "fork") } @@ -184,3 +184,12 @@ func displayResults(io *iostreams.IOStreams, results search.RepositoriesResult) } return tp.Render() } + +func visibilityLabel(repo search.Repository) string { + if repo.Visibility != "" { + return repo.Visibility + } else if repo.IsPrivate { + return "private" + } + return "public" +} From aff26cbcfc93cec24707942e8eb4b02966472458 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 7 Jun 2022 17:51:44 +0200 Subject: [PATCH 3/5] Simplify repo list color check --- pkg/cmd/repo/list/list.go | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/pkg/cmd/repo/list/list.go b/pkg/cmd/repo/list/list.go index bfd14d45b..a526594bd 100644 --- a/pkg/cmd/repo/list/list.go +++ b/pkg/cmd/repo/list/list.go @@ -173,9 +173,7 @@ func listRun(opts *ListOptions) error { info := repoInfo(repo) infoColor := cs.Gray - if (repo.Visibility != "" && - repo.Visibility != "PUBLIC") || - repo.IsPrivate { + if repo.IsPrivate { infoColor = cs.Yellow } From 80f130184ce0f87de4ad6eac59f6f7db07100980 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 7 Jun 2022 17:54:52 +0200 Subject: [PATCH 4/5] repo edit: fix interactive mode in GHES < 3.3 --- .../featuredetection/feature_detection.go | 5 +++++ .../feature_detection_test.go | 18 +++++++++++++++++ pkg/cmd/repo/edit/edit.go | 20 +++++++++++++++++-- 3 files changed, 41 insertions(+), 2 deletions(-) diff --git a/internal/featuredetection/feature_detection.go b/internal/featuredetection/feature_detection.go index 15eec5852..17254bb4f 100644 --- a/internal/featuredetection/feature_detection.go +++ b/internal/featuredetection/feature_detection.go @@ -37,6 +37,7 @@ type RepositoryFeatures struct { IssueTemplateQuery bool PullRequestTemplateQuery bool VisibilityField bool + AutoMerge bool } var allRepositoryFeatures = RepositoryFeatures{ @@ -44,6 +45,7 @@ var allRepositoryFeatures = RepositoryFeatures{ IssueTemplateQuery: true, PullRequestTemplateQuery: true, VisibilityField: true, + AutoMerge: true, } type detector struct { @@ -107,6 +109,9 @@ func (d *detector) RepositoryFeatures() (RepositoryFeatures, error) { if field.Name == "visibility" { features.VisibilityField = true } + if field.Name == "autoMergeAllowed" { + features.AutoMerge = true + } } return features, nil diff --git a/internal/featuredetection/feature_detection_test.go b/internal/featuredetection/feature_detection_test.go index 97ad0c0b8..b7aa2673f 100644 --- a/internal/featuredetection/feature_detection_test.go +++ b/internal/featuredetection/feature_detection_test.go @@ -73,6 +73,7 @@ func TestRepositoryFeatures(t *testing.T) { IssueTemplateQuery: true, PullRequestTemplateQuery: true, VisibilityField: true, + AutoMerge: true, }, wantErr: false, }, @@ -123,6 +124,23 @@ func TestRepositoryFeatures(t *testing.T) { }, wantErr: false, }, + { + name: "GHE has automerge field", + hostname: "git.my.org", + queryResponse: map[string]string{ + `query Repository_fields\b`: heredoc.Doc(` + { "data": { "Repository": { "fields": [ + {"name": "autoMergeAllowed"} + ] } } } + `), + }, + wantFeatures: RepositoryFeatures{ + IssueTemplateMutation: true, + IssueTemplateQuery: true, + AutoMerge: true, + }, + wantErr: false, + }, } for _, tt := range tests { diff --git a/pkg/cmd/repo/edit/edit.go b/pkg/cmd/repo/edit/edit.go index 953b69dbb..176de1b39 100644 --- a/pkg/cmd/repo/edit/edit.go +++ b/pkg/cmd/repo/edit/edit.go @@ -12,6 +12,7 @@ import ( "github.com/AlecAivazis/survey/v2" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" + fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmdutil" @@ -48,6 +49,7 @@ type EditOptions struct { AddTopics []string RemoveTopics []string InteractiveMode bool + Detector fd.Detector // Cache of current repo topics to avoid retrieving them // in multiple flows. topicsCache []string @@ -158,9 +160,17 @@ func editRun(ctx context.Context, opts *EditOptions) error { repo := opts.Repository if opts.InteractiveMode { + detector := opts.Detector + if detector == nil { + detector = fd.NewDetector(opts.HTTPClient, repo.RepoHost()) + } + repoFeatures, err := detector.RepositoryFeatures() + if err != nil { + return err + } + apiClient := api.NewClientFromHTTP(opts.HTTPClient) fieldsToRetrieve := []string{ - "autoMergeAllowed", "defaultBranchRef", "deleteBranchOnMerge", "description", @@ -174,8 +184,14 @@ func editRun(ctx context.Context, opts *EditOptions) error { "rebaseMergeAllowed", "repositoryTopics", "squashMergeAllowed", - "visibility", } + if repoFeatures.VisibilityField { + fieldsToRetrieve = append(fieldsToRetrieve, "visibility") + } + if repoFeatures.AutoMerge { + fieldsToRetrieve = append(fieldsToRetrieve, "autoMergeAllowed") + } + opts.IO.StartProgressIndicator() fetchedRepo, err := api.FetchRepository(apiClient, opts.Repository, fieldsToRetrieve) opts.IO.StopProgressIndicator() From f184d7ec5835fab460aadf336dfc98a994c370a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Tue, 7 Jun 2022 17:56:06 +0200 Subject: [PATCH 5/5] pr create: allow forking repositories with INTERNAL visibility The IsPrivate field of "internal" repositories is always true, but those repositories aren't truly private and absolutely can be forked. We shouldn't be checking for platform permissions in the client anyway, so let's just drop this check and have the platform decide whether this is a valid operation. --- pkg/cmd/pr/create/create.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 1c65046d5..f5691a563 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -588,9 +588,6 @@ func NewCreateContext(opts *CreateOptions) (*CreateContext, error) { return nil, cmdutil.CancelError } else { // "Create a fork of ..." - if baseRepo.IsPrivate { - return nil, fmt.Errorf("cannot fork private repository %s", ghrepo.FullName(baseRepo)) - } headBranchLabel = fmt.Sprintf("%s:%s", currentLogin, headBranch) } }