Skip to content

Feature detect v1 projects on non web-mode issue create #10815

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
williammartin merged 1 commit into from
Apr 23, 2025
Merged

Feature detect v1 projects on non web-mode issue create #10815

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 71 additions & 19 deletions api/queries_repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -863,7 +863,8 @@ type RepoMetadataInput struct {
Assignees bool
Reviewers bool
Labels bool
Projects bool
ProjectsV1 bool
ProjectsV2 bool
Milestones bool
}

Expand All @@ -882,6 +883,7 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput
return err
})
}

if input.Reviewers {
g.Go(func() error {
teams, err := OrganizationTeams(client, repo)
Expand All @@ -894,6 +896,7 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput
return nil
})
}

if input.Reviewers {
g.Go(func() error {
login, err := CurrentLoginName(client, repo.RepoHost())
Expand All @@ -904,6 +907,7 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput
return err
})
}

if input.Labels {
g.Go(func() error {
labels, err := RepoLabels(client, repo)
Expand All @@ -914,13 +918,23 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput
return err
})
}
if input.Projects {

if input.ProjectsV1 {
g.Go(func() error {
var err error
result.Projects, err = v1Projects(client, repo)
Copy link
Member

@babakks babakks Apr 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this field (i.e., RepoMetadataResult.Projects) be renamed to ProjectsV1? I know this might affect multiple places, but it will surely makes things less confusing.

Copy link
Member Author

@williammartin williammartin Apr 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is a good idea. Will address after stack is merged.

return err
})
}

if input.ProjectsV2 {
g.Go(func() error {
var err error
result.Projects, result.ProjectsV2, err = relevantProjects(client, repo)
result.ProjectsV2, err = v2Projects(client, repo)
return err
})
}

if input.Milestones {
g.Go(func() error {
milestones, err := RepoMilestones(client, repo, "open")
Expand All @@ -943,7 +957,8 @@ type RepoResolveInput struct {
Assignees []string
Reviewers []string
Labels []string
Projects []string
ProjectsV1 bool
Copy link
Member Author

@williammartin williammartin Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, in my opinion, the previous approach of "accepting aggregate projects" and "returning v1 and v2 separately" signified something funny. The result was that we'd have to prop drill the support variable everywhere like so: trunk...cli-9430#diff-5c07fcc22c80bad9356b4c3e1c794101855bcac70b031572c2084dc6d2023abaR872

I think it makes a lot more sense to make the decision as high up the stack as we can and then use types to correctly model expecations.

Not also that this changed to a bool because the values were never used.

ProjectsV2 bool
Milestones []string
}

Expand All @@ -970,7 +985,8 @@ func RepoResolveMetadataIDs(client *Client, repo ghrepo.Interface, input RepoRes

// there is no way to look up projects nor milestones by name, so preload them all
mi := RepoMetadataInput{
Projects: len(input.Projects) > 0,
ProjectsV1: input.ProjectsV1,
ProjectsV2: input.ProjectsV2,
Milestones: len(input.Milestones) > 0,
}
result, err := RepoMetadata(client, repo, mi)
Expand Down Expand Up @@ -1245,18 +1261,12 @@ func ProjectNamesToPaths(client *Client, repo ghrepo.Interface, projectNames []s
return ProjectsToPaths(projects, projectsV2, projectNames)
}

// RelevantProjects retrieves set of Projects and ProjectsV2 relevant to given repository:
// v1Projects retrieves set of RepoProjects relevant to given repository:
// - Projects for repository
// - Projects for repository organization, if it belongs to one
// - ProjectsV2 owned by current user
// - ProjectsV2 linked to repository
// - ProjectsV2 owned by repository organization, if it belongs to one
func relevantProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, []ProjectV2, error) {
func v1Projects(client *Client, repo ghrepo.Interface) ([]RepoProject, error) {
Copy link
Member Author

@williammartin williammartin Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The v1Projects and v2Projects functions are copy / pasted from relevantProjects. I cannot remove relevantProjects until I tackle web mode which has another code path to get there. It will go away. However, it now directly calls these two functions.

Copy link
Member Author

@williammartin williammartin Apr 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It goes away https://github.com/cli/cli/pull/10818/files#diff-5c07fcc22c80bad9356b4c3e1c794101855bcac70b031572c2084dc6d2023abaL1375

var repoProjects []RepoProject
var orgProjects []RepoProject
var userProjectsV2 []ProjectV2
var repoProjectsV2 []ProjectV2
var orgProjectsV2 []ProjectV2

g, _ := errgroup.WithContext(context.Background())

Expand All @@ -1268,6 +1278,7 @@ func relevantProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, []P
}
return err
})

g.Go(func() error {
var err error
orgProjects, err = OrganizationProjects(client, repo)
Expand All @@ -1277,6 +1288,29 @@ func relevantProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, []P
}
return nil
})

if err := g.Wait(); err != nil {
return nil, err
}

projects := make([]RepoProject, 0, len(repoProjects)+len(orgProjects))
projects = append(projects, repoProjects...)
projects = append(projects, orgProjects...)

return projects, nil
}

// v2Projects retrieves set of ProjectV2 relevant to given repository:
// - ProjectsV2 owned by current user
// - ProjectsV2 linked to repository
// - ProjectsV2 owned by repository organization, if it belongs to one
func v2Projects(client *Client, repo ghrepo.Interface) ([]ProjectV2, error) {
var userProjectsV2 []ProjectV2
var repoProjectsV2 []ProjectV2
var orgProjectsV2 []ProjectV2

g, _ := errgroup.WithContext(context.Background())

g.Go(func() error {
var err error
userProjectsV2, err = CurrentUserProjectsV2(client, repo.RepoHost())
Expand All @@ -1286,6 +1320,7 @@ func relevantProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, []P
}
return nil
})

g.Go(func() error {
var err error
repoProjectsV2, err = RepoProjectsV2(client, repo)
Expand All @@ -1295,6 +1330,7 @@ func relevantProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, []P
}
return nil
})

g.Go(func() error {
var err error
orgProjectsV2, err = OrganizationProjectsV2(client, repo)
Expand All @@ -1308,13 +1344,9 @@ func relevantProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, []P
})

if err := g.Wait(); err != nil {
return nil, nil, err
return nil, err
}

projects := make([]RepoProject, 0, len(repoProjects)+len(orgProjects))
projects = append(projects, repoProjects...)
projects = append(projects, orgProjects...)

// ProjectV2 might appear across multiple queries so use a map to keep them deduplicated.
m := make(map[string]ProjectV2, len(userProjectsV2)+len(repoProjectsV2)+len(orgProjectsV2))
for _, p := range userProjectsV2 {
Expand All @@ -1331,7 +1363,27 @@ func relevantProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, []P
projectsV2 = append(projectsV2, p)
}

return projects, projectsV2, nil
return projectsV2, nil
}

// relevantProjects retrieves set of Project or ProjectV2 relevant to given repository:
// - Projects for repository
// - Projects for repository organization, if it belongs to one
// - ProjectsV2 owned by current user
// - ProjectsV2 linked to repository
// - ProjectsV2 owned by repository organization, if it belongs to one
func relevantProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, []ProjectV2, error) {
v1Projects, err := v1Projects(client, repo)
if err != nil {
return nil, nil, err
}

v2Projects, err := v2Projects(client, repo)
if err != nil {
return nil, nil, err
}

return v1Projects, v2Projects, nil
}

func CreateRepoTransformToV4(apiClient *Client, hostname string, method string, path string, body io.Reader) (*Repository, error) {
Expand Down
3 changes: 2 additions & 1 deletion api/queries_repo_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ func Test_RepoMetadata(t *testing.T) {
Assignees: true,
Reviewers: true,
Labels: true,
Projects: true,
ProjectsV1: true,
ProjectsV2: true,
Milestones: true,
}

Expand Down
27 changes: 20 additions & 7 deletions pkg/cmd/issue/create/create.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,12 @@ import (
"errors"
"fmt"
"net/http"
"time"

"github.com/MakeNowJust/heredoc"
"github.com/cli/cli/v2/api"
"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"
Expand All @@ -24,6 +26,7 @@ type CreateOptions struct {
BaseRepo func() (ghrepo.Interface, error)
Browser browser.Browser
Prompter prShared.Prompt
Detector fd.Detector
TitledEditSurvey func(string, string) (string, string, error)

RootDirOverride string
Expand All @@ -46,11 +49,12 @@ type CreateOptions struct {

func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Command {
opts := &CreateOptions{
IO: f.IOStreams,
HttpClient: f.HttpClient,
Config: f.Config,
Browser: f.Browser,
Prompter: f.Prompter,
IO: f.IOStreams,
HttpClient: f.HttpClient,
Config: f.Config,
Browser: f.Browser,
Prompter: f.Prompter,

TitledEditSurvey: prShared.TitledEditSurvey(&prShared.UserEditor{Config: f.Config, IO: f.IOStreams}),
}

Expand Down Expand Up @@ -146,6 +150,15 @@ func createRun(opts *CreateOptions) (err error) {
return
}

// 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, baseRepo.RepoHost())
}

projectsV1Support := opts.Detector.ProjectsV1()

isTerminal := opts.IO.IsStdoutTTY()

var milestones []string
Expand Down Expand Up @@ -279,7 +292,7 @@ func createRun(opts *CreateOptions) (err error) {
Repo: baseRepo,
State: &tb,
}
err = prShared.MetadataSurvey(opts.Prompter, opts.IO, baseRepo, fetcher, &tb)
err = prShared.MetadataSurvey(opts.Prompter, opts.IO, baseRepo, fetcher, &tb, projectsV1Support)
Copy link
Member Author

@williammartin williammartin Apr 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block is untested, however I'm going to rely on the fact it's a simple value pass and the MetadataSurvey tests to avoid blowing up this work: https://github.com/cli/cli/pull/10815/files#diff-e76ab3c0932c8cf32002e9ad1bd0c61a3fd6f26d9bdcbbc65fdb30ec14e41a41R133-R175

if err != nil {
return
}
Expand Down Expand Up @@ -335,7 +348,7 @@ func createRun(opts *CreateOptions) (err error) {
params["issueTemplate"] = templateNameForSubmit
}

err = prShared.AddMetadataToIssueParams(apiClient, baseRepo, params, &tb)
err = prShared.AddMetadataToIssueParams(apiClient, baseRepo, params, &tb, projectsV1Support)
if err != nil {
return
}
Expand Down
78 changes: 78 additions & 0 deletions pkg/cmd/issue/create/create_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"github.com/MakeNowJust/heredoc"
"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"
Expand Down Expand Up @@ -521,6 +522,7 @@ func runCommandWithRootDirOverridden(rt http.RoundTripper, isTTY bool, cli strin

cmd := NewCmdCreate(factory, func(opts *CreateOptions) error {
opts.RootDirOverride = rootDir
opts.Detector = &fd.EnabledDetectorMock{}
return createRun(opts)
})

Expand Down Expand Up @@ -1026,3 +1028,79 @@ func TestIssueCreate_projectsV2(t *testing.T) {

assert.Equal(t, "https://github.com/OWNER/REPO/issues/12\n", output.String())
}

// 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, ""),
)
Comment on lines +1042 to +1047
Copy link
Member

@babakks babakks Apr 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great comments here. Thanks!


_, cmdTeardown := run.Stub()
defer cmdTeardown(t)

// 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{
IO: ios,
HttpClient: func() (*http.Client, error) {
return &http.Client{Transport: reg}, nil
},
BaseRepo: func() (ghrepo.Interface, error) {
return ghrepo.New("OWNER", "REPO"), nil
},

Detector: &fd.EnabledDetectorMock{},
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\(`))

_, cmdTeardown := run.Stub()
defer cmdTeardown(t)

// Ignore the error because we're not really interested in it.
_ = createRun(&CreateOptions{
IO: ios,
HttpClient: func() (*http.Client, error) {
return &http.Client{Transport: reg}, nil
},
BaseRepo: func() (ghrepo.Interface, error) {
return ghrepo.New("OWNER", "REPO"), nil
},

Detector: &fd.DisabledDetectorMock{},
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)
})
})
}
Loading
Loading