From e995a873cb7ad4be48c1ab1fbf51a85b7d0280c4 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 1 May 2025 15:58:48 +0200 Subject: [PATCH] Feature detect v1 projects on non-interactive pr create --- api/queries_repo.go | 41 ++++++----- api/queries_repo_test.go | 8 +- pkg/cmd/pr/create/create.go | 30 ++++++-- pkg/cmd/pr/create/create_test.go | 121 ++++++++++++++++++++++++++++++- pkg/cmd/pr/shared/editable.go | 6 +- pkg/cmd/pr/shared/params.go | 4 +- 6 files changed, 174 insertions(+), 36 deletions(-) diff --git a/api/queries_repo.go b/api/queries_repo.go index 27e21eb32..93a32d80c 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -738,34 +738,37 @@ func (m *RepoMetadataResult) LabelsToIDs(names []string) ([]string, error) { return ids, nil } -// ProjectsToIDs returns two arrays: +// ProjectsTitlesToIDs returns two arrays: // - the first contains IDs of projects V1 // - the second contains IDs of projects V2 // - if neither project V1 or project V2 can be found with a given name, then an error is returned -func (m *RepoMetadataResult) ProjectsToIDs(names []string) ([]string, []string, error) { +func (m *RepoMetadataResult) ProjectsTitlesToIDs(titles []string) ([]string, []string, error) { var ids []string var idsV2 []string - for _, projectName := range names { - id, found := m.projectNameToID(projectName) + for _, title := range titles { + id, found := m.v1ProjectNameToID(title) if found { ids = append(ids, id) continue } - idV2, found := m.projectV2TitleToID(projectName) + idV2, found := m.v2ProjectTitleToID(title) if found { idsV2 = append(idsV2, idV2) continue } - return nil, nil, fmt.Errorf("'%s' not found", projectName) + return nil, nil, fmt.Errorf("'%s' not found", title) } return ids, idsV2, nil } -func (m *RepoMetadataResult) projectNameToID(projectName string) (string, bool) { +// We use the word "titles" when referring to v1 and v2 projects. +// In reality, v1 projects really have "names", so there is a bit of a +// mismatch we just need to gloss over. +func (m *RepoMetadataResult) v1ProjectNameToID(name string) (string, bool) { for _, p := range m.Projects { - if strings.EqualFold(projectName, p.Name) { + if strings.EqualFold(name, p.Name) { return p.ID, true } } @@ -773,9 +776,9 @@ func (m *RepoMetadataResult) projectNameToID(projectName string) (string, bool) return "", false } -func (m *RepoMetadataResult) projectV2TitleToID(projectTitle string) (string, bool) { +func (m *RepoMetadataResult) v2ProjectTitleToID(title string) (string, bool) { for _, p := range m.ProjectsV2 { - if strings.EqualFold(projectTitle, p.Title) { + if strings.EqualFold(title, p.Title) { return p.ID, true } } @@ -783,8 +786,8 @@ func (m *RepoMetadataResult) projectV2TitleToID(projectTitle string) (string, bo return "", false } -func ProjectNamesToPaths(client *Client, repo ghrepo.Interface, projectNames []string, projectsV1Support gh.ProjectsV1Support) ([]string, error) { - paths := make([]string, 0, len(projectNames)) +func ProjectTitlesToPaths(client *Client, repo ghrepo.Interface, titles []string, projectsV1Support gh.ProjectsV1Support) ([]string, error) { + paths := make([]string, 0, len(titles)) matchedPaths := map[string]struct{}{} // TODO: ProjectsV1Cleanup @@ -796,9 +799,9 @@ func ProjectNamesToPaths(client *Client, repo ghrepo.Interface, projectNames []s return nil, err } - for _, projectName := range projectNames { + for _, title := range titles { for _, p := range v1Projects { - if strings.EqualFold(projectName, p.Name) { + if strings.EqualFold(title, p.Name) { pathParts := strings.Split(p.ResourcePath, "/") var path string if pathParts[1] == "orgs" || pathParts[1] == "users" { @@ -807,7 +810,7 @@ func ProjectNamesToPaths(client *Client, repo ghrepo.Interface, projectNames []s path = fmt.Sprintf("%s/%s/%s", pathParts[1], pathParts[2], pathParts[4]) } paths = append(paths, path) - matchedPaths[projectName] = struct{}{} + matchedPaths[title] = struct{}{} break } } @@ -820,15 +823,15 @@ func ProjectNamesToPaths(client *Client, repo ghrepo.Interface, projectNames []s return nil, err } - for _, projectName := range projectNames { + for _, title := range titles { // If we already found a v1 project with this name, skip it - if _, ok := matchedPaths[projectName]; ok { + if _, ok := matchedPaths[title]; ok { continue } found := false for _, p := range v2Projects { - if strings.EqualFold(projectName, p.Title) { + if strings.EqualFold(title, p.Title) { pathParts := strings.Split(p.ResourcePath, "/") var path string if pathParts[1] == "orgs" || pathParts[1] == "users" { @@ -843,7 +846,7 @@ func ProjectNamesToPaths(client *Client, repo ghrepo.Interface, projectNames []s } if !found { - return nil, fmt.Errorf("'%s' not found", projectName) + return nil, fmt.Errorf("'%s' not found", title) } } diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index 72ed35776..01fc7a4c7 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -187,7 +187,7 @@ func Test_RepoMetadata(t *testing.T) { expectedProjectIDs := []string{"TRIAGEID", "ROADMAPID"} expectedProjectV2IDs := []string{"TRIAGEV2ID", "ROADMAPV2ID", "MONALISAV2ID"} - projectIDs, projectV2IDs, err := result.ProjectsToIDs([]string{"triage", "roadmap", "triagev2", "roadmapv2", "monalisav2"}) + projectIDs, projectV2IDs, err := result.ProjectsTitlesToIDs([]string{"triage", "roadmap", "triagev2", "roadmapv2", "monalisav2"}) if err != nil { t.Errorf("error resolving projects: %v", err) } @@ -273,7 +273,7 @@ func Test_ProjectNamesToPaths(t *testing.T) { } } } } `)) - projectPaths, err := ProjectNamesToPaths(client, repo, []string{"Triage", "Roadmap", "TriageV2", "RoadmapV2", "MonalisaV2"}, gh.ProjectsV1Supported) + projectPaths, err := ProjectTitlesToPaths(client, repo, []string{"Triage", "Roadmap", "TriageV2", "RoadmapV2", "MonalisaV2"}, gh.ProjectsV1Supported) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -331,7 +331,7 @@ func Test_ProjectNamesToPaths(t *testing.T) { } } } } `)) - projectPaths, err := ProjectNamesToPaths(client, repo, []string{"TriageV2", "RoadmapV2", "MonalisaV2"}, gh.ProjectsV1Unsupported) + projectPaths, err := ProjectTitlesToPaths(client, repo, []string{"TriageV2", "RoadmapV2", "MonalisaV2"}, gh.ProjectsV1Unsupported) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -374,7 +374,7 @@ func Test_ProjectNamesToPaths(t *testing.T) { } } } } `)) - _, err := ProjectNamesToPaths(client, repo, []string{"TriageV2"}, gh.ProjectsV1Unsupported) + _, err := ProjectTitlesToPaths(client, repo, []string{"TriageV2"}, gh.ProjectsV1Unsupported) require.Equal(t, err, fmt.Errorf("'TriageV2' not found")) }) } diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 7f960bce4..483b8246b 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -18,6 +18,7 @@ import ( ghContext "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/browser" + fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/text" @@ -31,6 +32,7 @@ import ( type CreateOptions struct { // This struct stores user input and factory functions + Detector fd.Detector HttpClient func() (*http.Client, error) GitClient *git.Client Config func() (gh.Config, error) @@ -363,6 +365,20 @@ func createRun(opts *CreateOptions) error { return err } + httpClient, err := opts.HttpClient() + if err != nil { + return err + } + + // TODO projectsV1Deprecation + // Remove this section as we should no longer need to detect + if opts.Detector == nil { + cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24) + opts.Detector = fd.NewDetector(cachedClient, ctx.PRRefs.BaseRepo().RepoHost()) + } + + projectsV1Support := opts.Detector.ProjectsV1() + client := ctx.Client state, err := NewIssueState(*ctx, *opts) @@ -384,7 +400,7 @@ func createRun(opts *CreateOptions) error { if err != nil { return err } - openURL, err = generateCompareURL(*ctx, *state) + openURL, err = generateCompareURL(*ctx, *state, gh.ProjectsV1Supported) if err != nil { return err } @@ -441,7 +457,7 @@ func createRun(opts *CreateOptions) error { return err } // TODO wm: revisit project support - return submitPR(*opts, *ctx, *state, gh.ProjectsV1Supported) + return submitPR(*opts, *ctx, *state, projectsV1Support) } if opts.RecoverFile != "" { @@ -518,7 +534,7 @@ func createRun(opts *CreateOptions) error { } } - openURL, err = generateCompareURL(*ctx, *state) + openURL, err = generateCompareURL(*ctx, *state, gh.ProjectsV1Supported) if err != nil { return err } @@ -568,12 +584,12 @@ func createRun(opts *CreateOptions) error { if action == shared.SubmitDraftAction { state.Draft = true // TODO wm: revisit project support - return submitPR(*opts, *ctx, *state, gh.ProjectsV1Supported) + return submitPR(*opts, *ctx, *state, projectsV1Support) } if action == shared.SubmitAction { // TODO wm: revisit project support - return submitPR(*opts, *ctx, *state, gh.ProjectsV1Supported) + return submitPR(*opts, *ctx, *state, projectsV1Support) } err = errors.New("expected to cancel, preview, or submit") @@ -1216,13 +1232,13 @@ func handlePush(opts CreateOptions, ctx CreateContext) error { return pushBranch() } -func generateCompareURL(ctx CreateContext, state shared.IssueMetadataState) (string, error) { +func generateCompareURL(ctx CreateContext, state shared.IssueMetadataState, projectsV1Support gh.ProjectsV1Support) (string, error) { u := ghrepo.GenerateRepoURL( ctx.PRRefs.BaseRepo(), "compare/%s...%s?expand=1", url.PathEscape(ctx.PRRefs.BaseRef()), url.PathEscape(ctx.PRRefs.QualifiedHeadRef())) // TODO wm: revisit project support - url, err := shared.WithPrAndIssueQueryParams(ctx.Client, ctx.PRRefs.BaseRepo(), u, state, gh.ProjectsV1Supported) + url, err := shared.WithPrAndIssueQueryParams(ctx.Client, ctx.PRRefs.BaseRepo(), u, state, projectsV1Support) if err != nil { return "", err } diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index 2a88b5eee..f3b99bc89 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -15,6 +15,7 @@ import ( "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/browser" "github.com/cli/cli/v2/internal/config" + fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/prompter" @@ -1618,6 +1619,7 @@ func Test_createRun(t *testing.T) { } opts := CreateOptions{} + opts.Detector = &fd.EnabledDetectorMock{} opts.Prompter = pm ios, _, stdout, stderr := iostreams.Test() @@ -1941,7 +1943,8 @@ func Test_generateCompareURL(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := generateCompareURL(tt.ctx, tt.state) + // TODO wm: projects v1 support? + got, err := generateCompareURL(tt.ctx, tt.state, gh.ProjectsV1Supported) if (err != nil) != tt.wantErr { t.Errorf("generateCompareURL() error = %v, wantErr %v", err, tt.wantErr) return @@ -2009,3 +2012,119 @@ func mockRetrieveProjects(_ *testing.T, reg *httpmock.Registry) { } // TODO interactive metadata tests once: 1) we have test utils for Prompter and 2) metadata questions use Prompter + +// TODO projectsV1Deprecation +// Remove this test. +func TestProjectsV1Deprecation(t *testing.T) { + + t.Run("non-interactive submission", func(t *testing.T) { + t.Run("when projects v1 is supported, queries for it", func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + + reg := &httpmock.Registry{} + reg.StubRepoInfoResponse("OWNER", "REPO", "main") + reg.Register( + // ( is required to avoid matching projectsV2 + httpmock.GraphQL(`projects\(`), + // Simulate a GraphQL error to early exit the test. + httpmock.StatusStringResponse(500, ""), + ) + + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + cs.Register(`git config --get-regexp \^branch\\\..+\\\.\(remote\|merge\|pushremote\|gh-merge-base\)\$`, 0, "") + + // Ignore the error because we have no way to really stub it without + // fully stubbing a GQL error structure in the request body. + _ = createRun(&CreateOptions{ + Detector: &fd.EnabledDetectorMock{}, + IO: ios, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + GitClient: &git.Client{ + GhPath: "some/path/gh", + GitPath: "some/path/git", + }, + Remotes: func() (context.Remotes, error) { + return context.Remotes{ + { + Remote: &git.Remote{ + Name: "upstream", + Resolved: "base", + }, + Repo: ghrepo.New("OWNER", "REPO"), + }, + }, nil + }, + Finder: shared.NewMockFinder("feature", nil, nil), + + HeadBranch: "feature", + + TitleProvided: true, + BodyProvided: true, + Title: "Test Title", + Body: "Test Body", + + // Required to force a lookup of projects + Projects: []string{"Project"}, + }) + + // Verify that our request contained projects + reg.Verify(t) + }) + + t.Run("when projects v1 is not supported, does not query for it", func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + + reg := &httpmock.Registry{} + reg.StubRepoInfoResponse("OWNER", "REPO", "main") + // ( is required to avoid matching projectsV2 + reg.Exclude(t, httpmock.GraphQL(`projects\(`)) + + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + cs.Register(`git config --get-regexp \^branch\\\..+\\\.\(remote\|merge\|pushremote\|gh-merge-base\)\$`, 0, "") + + // Ignore the error because we're not really interested in it. + _ = createRun(&CreateOptions{ + Detector: &fd.DisabledDetectorMock{}, + IO: ios, + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + GitClient: &git.Client{ + GhPath: "some/path/gh", + GitPath: "some/path/git", + }, + Remotes: func() (context.Remotes, error) { + return context.Remotes{ + { + Remote: &git.Remote{ + Name: "upstream", + Resolved: "base", + }, + Repo: ghrepo.New("OWNER", "REPO"), + }, + }, nil + }, + Finder: shared.NewMockFinder("feature", nil, nil), + + HeadBranch: "feature", + + TitleProvided: true, + BodyProvided: true, + Title: "Test Title", + Body: "Test Body", + + // Required to force a lookup of projects + Projects: []string{"Project"}, + }) + + // Verify that our request contained projectCards + reg.Verify(t) + }) + }) +} diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index 0bebb999a..e73b3c294 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -137,7 +137,7 @@ func (e Editable) ProjectIds() (*[]string, error) { s.RemoveValues(e.Projects.Remove) e.Projects.Value = s.ToSlice() } - p, _, err := e.Metadata.ProjectsToIDs(e.Projects.Value) + p, _, err := e.Metadata.ProjectsTitlesToIDs(e.Projects.Value) return &p, err } @@ -171,14 +171,14 @@ func (e Editable) ProjectV2Ids() (*[]string, *[]string, error) { var err error if addTitles.Len() > 0 { - _, addIds, err = e.Metadata.ProjectsToIDs(addTitles.ToSlice()) + _, addIds, err = e.Metadata.ProjectsTitlesToIDs(addTitles.ToSlice()) if err != nil { return nil, nil, err } } if removeTitles.Len() > 0 { - _, removeIds, err = e.Metadata.ProjectsToIDs(removeTitles.ToSlice()) + _, removeIds, err = e.Metadata.ProjectsTitlesToIDs(removeTitles.ToSlice()) if err != nil { return nil, nil, err } diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 4f36a80aa..08968939d 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -36,7 +36,7 @@ func WithPrAndIssueQueryParams(client *api.Client, baseRepo ghrepo.Interface, ba q.Set("labels", strings.Join(state.Labels, ",")) } if len(state.ProjectTitles) > 0 { - projectPaths, err := api.ProjectNamesToPaths(client, baseRepo, state.ProjectTitles, projectsV1Support) + projectPaths, err := api.ProjectTitlesToPaths(client, baseRepo, state.ProjectTitles, projectsV1Support) if err != nil { return "", fmt.Errorf("could not add to project: %w", err) } @@ -119,7 +119,7 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par } params["labelIds"] = labelIDs - projectIDs, projectV2IDs, err := tb.MetadataResult.ProjectsToIDs(tb.ProjectTitles) + projectIDs, projectV2IDs, err := tb.MetadataResult.ProjectsTitlesToIDs(tb.ProjectTitles) if err != nil { return fmt.Errorf("could not add to project: %w", err) }