diff --git a/acceptance/testdata/pr/pr-create-guesses-remote-from-sha-with-branch-name-slash.txtar b/acceptance/testdata/pr/pr-create-guesses-remote-from-sha-with-branch-name-slash.txtar new file mode 100644 index 000000000..542579b0a --- /dev/null +++ b/acceptance/testdata/pr/pr-create-guesses-remote-from-sha-with-branch-name-slash.txtar @@ -0,0 +1,50 @@ +skip 'it creates a fork owned by the user running the test' + +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} +env FORK=${REPO}-fork + +# Use gh as a credential helper +exec gh auth setup-git + +# Get the current username for the fork owner +exec gh api user --jq .login +stdout2env USER + +# Create a repository with a file so it has a default branch +exec gh repo create ${ORG}/${REPO} --add-readme --private + +# Defer repo cleanup +defer gh repo delete --yes ${ORG}/${REPO} + +# Create a user fork of repository. This will be owned by USER. +exec gh repo fork ${ORG}/${REPO} --fork-name ${FORK} +sleep 5 + +# Defer repo cleanup of fork +defer gh repo delete --yes ${USER}/${FORK} + +# Retrieve fork repository information +exec gh repo view ${USER}/${FORK} --json id --jq '.id' +stdout2env FORK_ID + +exec gh repo clone ${USER}/${FORK} +cd ${FORK} + +# Prepare a branch to commit +exec git checkout -b feature/branch +exec git commit --allow-empty -m 'Upstream Commit' +# Push without setting an upstream (-u or config) +exec git push upstream feature/branch + +# Prepare an additional commit +exec git commit --allow-empty -m 'Fork Commit' +# Push without setting an upstream (-u or config) +exec git push origin feature/branch + +# Create the PR +exec gh pr create --title 'Feature Title' --body 'Feature Body' +stdout https://${GH_HOST}/${ORG}/${REPO}/pull/1 + +# Check the PR is indeed created +exec gh pr view ${USER}:feature/branch --json headRefName,headRepository,baseRefName,isCrossRepository +stdout {"baseRefName":"main","headRefName":"feature/branch","headRepository":{"id":"${FORK_ID}","name":"${FORK}"},"isCrossRepository":true} diff --git a/acceptance/testdata/pr/pr-create-guesses-remote-from-sha.txtar b/acceptance/testdata/pr/pr-create-guesses-remote-from-sha.txtar index 52579b501..e263b0351 100644 --- a/acceptance/testdata/pr/pr-create-guesses-remote-from-sha.txtar +++ b/acceptance/testdata/pr/pr-create-guesses-remote-from-sha.txtar @@ -1,3 +1,5 @@ +skip 'it creates a fork owned by the user running the test' + env REPO=${SCRIPT_NAME}-${RANDOM_STRING} env FORK=${REPO}-fork diff --git a/acceptance/testdata/pr/pr-create-push-default-upstream-no-merge-ref-fork.txtar b/acceptance/testdata/pr/pr-create-push-default-upstream-no-merge-ref-fork.txtar new file mode 100644 index 000000000..0974f9225 --- /dev/null +++ b/acceptance/testdata/pr/pr-create-push-default-upstream-no-merge-ref-fork.txtar @@ -0,0 +1,50 @@ +skip 'it creates a fork owned by the user running the test' +skip 'this never worked, but could be fixed if we fixed show-refs' + +# Setup environment variables used for testscript +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} +env FORK=${REPO}-fork + +# Use gh as a credential helper +exec gh auth setup-git + +# Get the current username for the fork owner +exec gh api user --jq .login +stdout2env USER + +# Create a repository to act as upstream with a file so it has a default branch +exec gh repo create ${ORG}/${REPO} --add-readme --private + +# Defer repo cleanup of upstream +defer gh repo delete --yes ${ORG}/${REPO} + +# Create a user fork of repository. This will be owned by USER. +exec gh repo fork ${ORG}/${REPO} --fork-name ${FORK} +sleep 5 + +# Defer repo cleanup of fork +defer gh repo delete --yes ${USER}/${FORK} + +# Retrieve fork repository information +exec gh repo view ${USER}/${FORK} --json id --jq '.id' +stdout2env FORK_ID + +# Clone the repo +exec gh repo clone ${USER}/${FORK} +cd ${FORK} + +# Configure push.default so that it should use the merge ref +exec git config push.default upstream + +# But prepare a branch that doesn't have a tracking merge ref +exec git checkout -b feature-branch +exec git commit --allow-empty -m 'Empty Commit' +exec git push origin feature-branch + +# Create the PR +exec gh pr create --title 'Feature Title' --body 'Feature Body' +stdout https://${GH_HOST}/${ORG}/${REPO}/pull/1 + +# Assert that the PR was created with the correct head repository and refs +exec gh pr view --json headRefName,headRepository,baseRefName,isCrossRepository +stdout {"baseRefName":"main","headRefName":"feature-branch","headRepository":{"id":"${FORK_ID}","name":"${FORK}"},"isCrossRepository":true} diff --git a/acceptance/testdata/pr/pr-create-push-default-upstream-no-merge-ref.txtar b/acceptance/testdata/pr/pr-create-push-default-upstream-no-merge-ref.txtar new file mode 100644 index 000000000..90c5cde50 --- /dev/null +++ b/acceptance/testdata/pr/pr-create-push-default-upstream-no-merge-ref.txtar @@ -0,0 +1,33 @@ +skip 'it creates a fork owned by the user running the test' + +# Setup environment variables used for testscript +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + +# Use gh as a credential helper +exec gh auth setup-git + +# Get the current username for the fork owner +exec gh api user --jq .login +stdout2env USER + +# Create a repository to act as upstream with a file so it has a default branch +exec gh repo create ${ORG}/${REPO} --add-readme --private + +# Defer repo cleanup of upstream +defer gh repo delete --yes ${ORG}/${REPO} + +# Clone the repo +exec gh repo clone ${ORG}/${REPO} +cd ${REPO} + +# Configure push.default so that it should use the merge ref +exec git config push.default upstream + +# But prepare a branch that doesn't have a tracking merge ref +exec git checkout -b feature-branch +exec git commit --allow-empty -m 'Empty Commit' +exec git push origin feature-branch + +# Create the PR +exec gh pr create --title 'Feature Title' --body 'Feature Body' +stdout https://${GH_HOST}/${ORG}/${REPO}/pull/1 diff --git a/acceptance/testdata/pr/pr-create-remote-ref-with-branch-name-slash.txtar b/acceptance/testdata/pr/pr-create-remote-ref-with-branch-name-slash.txtar new file mode 100644 index 000000000..395fce86a --- /dev/null +++ b/acceptance/testdata/pr/pr-create-remote-ref-with-branch-name-slash.txtar @@ -0,0 +1,46 @@ +skip 'it creates a fork owned by the user running the test' + +# Setup environment variables used for testscript +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} +env FORK=${REPO}-fork + +# Use gh as a credential helper +exec gh auth setup-git + +# Get the current username for the fork owner +exec gh api user --jq .login +stdout2env USER + +# Create a repository to act as upstream with a file so it has a default branch +exec gh repo create ${ORG}/${REPO} --add-readme --private + +# Defer repo cleanup of upstream +defer gh repo delete --yes ${ORG}/${REPO} + +# Create a user fork of repository. This will be owned by USER. +exec gh repo fork ${ORG}/${REPO} --fork-name ${FORK} +sleep 5 + +# Defer repo cleanup of fork +defer gh repo delete --yes ${USER}/${FORK} + +# Retrieve fork repository information +exec gh repo view ${USER}/${FORK} --json id --jq '.id' +stdout2env FORK_ID + +# Clone the repo +exec gh repo clone ${USER}/${FORK} +cd ${FORK} + +# Prepare a branch where changes are pulled from the upstream default branch but pushed to fork +exec git checkout -b feature/branch +exec git commit --allow-empty -m 'Empty Commit' +exec git push -u origin feature/branch + +# Create the PR spanning upstream and fork repositories +exec gh pr create --title 'Feature Title' --body 'Feature Body' +stdout https://${GH_HOST}/${ORG}/${REPO}/pull/1 + +# Assert that the PR was created with the correct head repository and refs +exec gh pr view --json headRefName,headRepository,baseRefName,isCrossRepository +stdout {"baseRefName":"main","headRefName":"feature/branch","headRepository":{"id":"${FORK_ID}","name":"${FORK}"},"isCrossRepository":true} diff --git a/acceptance/testdata/repo/repo-set-default.txtar b/acceptance/testdata/repo/repo-set-default.txtar index 4f7fa3273..de4eda11f 100644 --- a/acceptance/testdata/repo/repo-set-default.txtar +++ b/acceptance/testdata/repo/repo-set-default.txtar @@ -7,7 +7,7 @@ defer gh repo delete --yes $ORG/$SCRIPT_NAME-$RANDOM_STRING # Ensure that no default is set cd $SCRIPT_NAME-$RANDOM_STRING exec gh repo set-default --view -stderr 'no default repository has been set; use `gh repo set-default` to select one' +stderr 'No default remote repository has been set. To learn more about the default repository, run: gh repo set-default --help' # Set the default exec gh repo set-default $ORG/$SCRIPT_NAME-$RANDOM_STRING diff --git a/api/queries_repo.go b/api/queries_repo.go index 53e6d879a..27e21eb32 100644 --- a/api/queries_repo.go +++ b/api/queries_repo.go @@ -12,6 +12,7 @@ import ( "strings" "time" + "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghinstance" "golang.org/x/sync/errgroup" @@ -782,35 +783,54 @@ func (m *RepoMetadataResult) projectV2TitleToID(projectTitle string) (string, bo return "", false } -func ProjectsToPaths(projects []RepoProject, projectsV2 []ProjectV2, names []string) ([]string, error) { - var paths []string - for _, projectName := range names { - found := false - for _, p := range projects { - if strings.EqualFold(projectName, p.Name) { - // format of ResourcePath: /OWNER/REPO/projects/PROJECT_NUMBER or /orgs/ORG/projects/PROJECT_NUMBER or /users/USER/projects/PROJECT_NUBER - // required format of path: OWNER/REPO/PROJECT_NUMBER or ORG/PROJECT_NUMBER or USER/PROJECT_NUMBER - var path string - pathParts := strings.Split(p.ResourcePath, "/") - if pathParts[1] == "orgs" || pathParts[1] == "users" { - path = fmt.Sprintf("%s/%s", pathParts[2], pathParts[4]) - } else { - path = fmt.Sprintf("%s/%s/%s", pathParts[1], pathParts[2], pathParts[4]) +func ProjectNamesToPaths(client *Client, repo ghrepo.Interface, projectNames []string, projectsV1Support gh.ProjectsV1Support) ([]string, error) { + paths := make([]string, 0, len(projectNames)) + matchedPaths := map[string]struct{}{} + + // TODO: ProjectsV1Cleanup + // At this point, we only know the names that the user has provided, so we can't push this conditional up the stack. + // First we'll try to match against v1 projects, if supported + if projectsV1Support == gh.ProjectsV1Supported { + v1Projects, err := v1Projects(client, repo) + if err != nil { + return nil, err + } + + for _, projectName := range projectNames { + for _, p := range v1Projects { + if strings.EqualFold(projectName, p.Name) { + pathParts := strings.Split(p.ResourcePath, "/") + var path string + if pathParts[1] == "orgs" || pathParts[1] == "users" { + path = fmt.Sprintf("%s/%s", pathParts[2], pathParts[4]) + } else { + path = fmt.Sprintf("%s/%s/%s", pathParts[1], pathParts[2], pathParts[4]) + } + paths = append(paths, path) + matchedPaths[projectName] = struct{}{} + break } - paths = append(paths, path) - found = true - break } } - if found { + } + + // Then we'll try to match against v2 projects + v2Projects, err := v2Projects(client, repo) + if err != nil { + return nil, err + } + + for _, projectName := range projectNames { + // If we already found a v1 project with this name, skip it + if _, ok := matchedPaths[projectName]; ok { continue } - for _, p := range projectsV2 { + + found := false + for _, p := range v2Projects { if strings.EqualFold(projectName, p.Title) { - // format of ResourcePath: /OWNER/REPO/projects/PROJECT_NUMBER or /orgs/ORG/projects/PROJECT_NUMBER or /users/USER/projects/PROJECT_NUBER - // required format of path: OWNER/REPO/PROJECT_NUMBER or ORG/PROJECT_NUMBER or USER/PROJECT_NUMBER - var path string pathParts := strings.Split(p.ResourcePath, "/") + var path string if pathParts[1] == "orgs" || pathParts[1] == "users" { path = fmt.Sprintf("%s/%s", pathParts[2], pathParts[4]) } else { @@ -821,10 +841,12 @@ func ProjectsToPaths(projects []RepoProject, projectsV2 []ProjectV2, names []str break } } + if !found { return nil, fmt.Errorf("'%s' not found", projectName) } } + return paths, nil } @@ -863,7 +885,8 @@ type RepoMetadataInput struct { Assignees bool Reviewers bool Labels bool - Projects bool + ProjectsV1 bool + ProjectsV2 bool Milestones bool } @@ -882,6 +905,7 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput return err }) } + if input.Reviewers { g.Go(func() error { teams, err := OrganizationTeams(client, repo) @@ -894,6 +918,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()) @@ -904,6 +929,7 @@ func RepoMetadata(client *Client, repo ghrepo.Interface, input RepoMetadataInput return err }) } + if input.Labels { g.Go(func() error { labels, err := RepoLabels(client, repo) @@ -914,13 +940,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, result.ProjectsV2, err = relevantProjects(client, repo) + result.Projects, err = v1Projects(client, repo) return err }) } + + if input.ProjectsV2 { + g.Go(func() error { + var err error + result.ProjectsV2, err = v2Projects(client, repo) + return err + }) + } + if input.Milestones { g.Go(func() error { milestones, err := RepoMilestones(client, repo, "open") @@ -943,7 +979,8 @@ type RepoResolveInput struct { Assignees []string Reviewers []string Labels []string - Projects []string + ProjectsV1 bool + ProjectsV2 bool Milestones []string } @@ -970,7 +1007,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) @@ -1237,26 +1275,12 @@ func RepoMilestones(client *Client, repo ghrepo.Interface, state string) ([]Repo return milestones, nil } -func ProjectNamesToPaths(client *Client, repo ghrepo.Interface, projectNames []string) ([]string, error) { - projects, projectsV2, err := relevantProjects(client, repo) - if err != nil { - return nil, err - } - 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) { var repoProjects []RepoProject var orgProjects []RepoProject - var userProjectsV2 []ProjectV2 - var repoProjectsV2 []ProjectV2 - var orgProjectsV2 []ProjectV2 g, _ := errgroup.WithContext(context.Background()) @@ -1268,6 +1292,7 @@ func relevantProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, []P } return err }) + g.Go(func() error { var err error orgProjects, err = OrganizationProjects(client, repo) @@ -1277,6 +1302,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()) @@ -1286,6 +1334,7 @@ func relevantProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, []P } return nil }) + g.Go(func() error { var err error repoProjectsV2, err = RepoProjectsV2(client, repo) @@ -1295,6 +1344,7 @@ func relevantProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, []P } return nil }) + g.Go(func() error { var err error orgProjectsV2, err = OrganizationProjectsV2(client, repo) @@ -1308,13 +1358,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 { @@ -1331,7 +1377,7 @@ func relevantProjects(client *Client, repo ghrepo.Interface) ([]RepoProject, []P projectsV2 = append(projectsV2, p) } - return projects, projectsV2, nil + return projectsV2, nil } func CreateRepoTransformToV4(apiClient *Client, hostname string, method string, path string, body io.Reader) (*Repository, error) { diff --git a/api/queries_repo_test.go b/api/queries_repo_test.go index 13aee459a..72ed35776 100644 --- a/api/queries_repo_test.go +++ b/api/queries_repo_test.go @@ -8,6 +8,7 @@ import ( "testing" "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/httpmock" "github.com/stretchr/testify/assert" @@ -44,7 +45,8 @@ func Test_RepoMetadata(t *testing.T) { Assignees: true, Reviewers: true, Labels: true, - Projects: true, + ProjectsV1: true, + ProjectsV2: true, Milestones: true, } @@ -211,37 +213,16 @@ func Test_RepoMetadata(t *testing.T) { } } -func Test_ProjectsToPaths(t *testing.T) { - expectedProjectPaths := []string{"OWNER/REPO/PROJECT_NUMBER", "ORG/PROJECT_NUMBER", "OWNER/REPO/PROJECT_NUMBER_2"} - projects := []RepoProject{ - {ID: "id1", Name: "My Project", ResourcePath: "/OWNER/REPO/projects/PROJECT_NUMBER"}, - {ID: "id2", Name: "Org Project", ResourcePath: "/orgs/ORG/projects/PROJECT_NUMBER"}, - {ID: "id3", Name: "Project", ResourcePath: "/orgs/ORG/projects/PROJECT_NUMBER_2"}, - } - projectsV2 := []ProjectV2{ - {ID: "id4", Title: "My Project V2", ResourcePath: "/OWNER/REPO/projects/PROJECT_NUMBER_2"}, - {ID: "id5", Title: "Org Project V2", ResourcePath: "/orgs/ORG/projects/PROJECT_NUMBER_3"}, - } - projectNames := []string{"My Project", "Org Project", "My Project V2"} - - projectPaths, err := ProjectsToPaths(projects, projectsV2, projectNames) - if err != nil { - t.Errorf("error resolving projects: %v", err) - } - if !sliceEqual(projectPaths, expectedProjectPaths) { - t.Errorf("expected projects %v, got %v", expectedProjectPaths, projectPaths) - } -} - func Test_ProjectNamesToPaths(t *testing.T) { - http := &httpmock.Registry{} - client := newTestClient(http) + t.Run("when projectsV1 is supported, requests them", func(t *testing.T) { + http := &httpmock.Registry{} + client := newTestClient(http) - repo, _ := ghrepo.FromFullName("OWNER/REPO") + repo, _ := ghrepo.FromFullName("OWNER/REPO") - http.Register( - httpmock.GraphQL(`query RepositoryProjectList\b`), - httpmock.StringResponse(` + http.Register( + httpmock.GraphQL(`query RepositoryProjectList\b`), + httpmock.StringResponse(` { "data": { "repository": { "projects": { "nodes": [ { "name": "Cleanup", "id": "CLEANUPID", "resourcePath": "/OWNER/REPO/projects/1" }, @@ -250,9 +231,9 @@ func Test_ProjectNamesToPaths(t *testing.T) { "pageInfo": { "hasNextPage": false } } } } } `)) - http.Register( - httpmock.GraphQL(`query OrganizationProjectList\b`), - httpmock.StringResponse(` + http.Register( + httpmock.GraphQL(`query OrganizationProjectList\b`), + httpmock.StringResponse(` { "data": { "organization": { "projects": { "nodes": [ { "name": "Triage", "id": "TRIAGEID", "resourcePath": "/orgs/ORG/projects/1" } @@ -260,9 +241,9 @@ func Test_ProjectNamesToPaths(t *testing.T) { "pageInfo": { "hasNextPage": false } } } } } `)) - http.Register( - httpmock.GraphQL(`query RepositoryProjectV2List\b`), - httpmock.StringResponse(` + http.Register( + httpmock.GraphQL(`query RepositoryProjectV2List\b`), + httpmock.StringResponse(` { "data": { "repository": { "projectsV2": { "nodes": [ { "title": "CleanupV2", "id": "CLEANUPV2ID", "resourcePath": "/OWNER/REPO/projects/3" }, @@ -271,9 +252,9 @@ func Test_ProjectNamesToPaths(t *testing.T) { "pageInfo": { "hasNextPage": false } } } } } `)) - http.Register( - httpmock.GraphQL(`query OrganizationProjectV2List\b`), - httpmock.StringResponse(` + http.Register( + httpmock.GraphQL(`query OrganizationProjectV2List\b`), + httpmock.StringResponse(` { "data": { "organization": { "projectsV2": { "nodes": [ { "title": "TriageV2", "id": "TRIAGEV2ID", "resourcePath": "/orgs/ORG/projects/2" } @@ -281,9 +262,9 @@ func Test_ProjectNamesToPaths(t *testing.T) { "pageInfo": { "hasNextPage": false } } } } } `)) - http.Register( - httpmock.GraphQL(`query UserProjectV2List\b`), - httpmock.StringResponse(` + http.Register( + httpmock.GraphQL(`query UserProjectV2List\b`), + httpmock.StringResponse(` { "data": { "viewer": { "projectsV2": { "nodes": [ { "title": "MonalisaV2", "id": "MONALISAV2ID", "resourcePath": "/users/MONALISA/projects/5" } @@ -292,15 +273,110 @@ func Test_ProjectNamesToPaths(t *testing.T) { } } } } `)) - projectPaths, err := ProjectNamesToPaths(client, repo, []string{"Triage", "Roadmap", "TriageV2", "RoadmapV2", "MonalisaV2"}) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } + projectPaths, err := ProjectNamesToPaths(client, repo, []string{"Triage", "Roadmap", "TriageV2", "RoadmapV2", "MonalisaV2"}, gh.ProjectsV1Supported) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } - expectedProjectPaths := []string{"ORG/1", "OWNER/REPO/2", "ORG/2", "OWNER/REPO/4", "MONALISA/5"} - if !sliceEqual(projectPaths, expectedProjectPaths) { - t.Errorf("expected projects paths %v, got %v", expectedProjectPaths, projectPaths) - } + expectedProjectPaths := []string{"ORG/1", "OWNER/REPO/2", "ORG/2", "OWNER/REPO/4", "MONALISA/5"} + if !sliceEqual(projectPaths, expectedProjectPaths) { + t.Errorf("expected projects paths %v, got %v", expectedProjectPaths, projectPaths) + } + }) + + t.Run("when projectsV1 is not supported, does not request them", func(t *testing.T) { + http := &httpmock.Registry{} + client := newTestClient(http) + + repo, _ := ghrepo.FromFullName("OWNER/REPO") + + http.Exclude( + t, + httpmock.GraphQL(`query RepositoryProjectList\b`), + ) + http.Exclude( + t, + httpmock.GraphQL(`query OrganizationProjectList\b`), + ) + + http.Register( + httpmock.GraphQL(`query RepositoryProjectV2List\b`), + httpmock.StringResponse(` + { "data": { "repository": { "projectsV2": { + "nodes": [ + { "title": "CleanupV2", "id": "CLEANUPV2ID", "resourcePath": "/OWNER/REPO/projects/3" }, + { "title": "RoadmapV2", "id": "ROADMAPV2ID", "resourcePath": "/OWNER/REPO/projects/4" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + http.Register( + httpmock.GraphQL(`query OrganizationProjectV2List\b`), + httpmock.StringResponse(` + { "data": { "organization": { "projectsV2": { + "nodes": [ + { "title": "TriageV2", "id": "TRIAGEV2ID", "resourcePath": "/orgs/ORG/projects/2" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + http.Register( + httpmock.GraphQL(`query UserProjectV2List\b`), + httpmock.StringResponse(` + { "data": { "viewer": { "projectsV2": { + "nodes": [ + { "title": "MonalisaV2", "id": "MONALISAV2ID", "resourcePath": "/users/MONALISA/projects/5" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + + projectPaths, err := ProjectNamesToPaths(client, repo, []string{"TriageV2", "RoadmapV2", "MonalisaV2"}, gh.ProjectsV1Unsupported) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + expectedProjectPaths := []string{"ORG/2", "OWNER/REPO/4", "MONALISA/5"} + if !sliceEqual(projectPaths, expectedProjectPaths) { + t.Errorf("expected projects paths %v, got %v", expectedProjectPaths, projectPaths) + } + }) + + t.Run("when a project is not found, returns an error", func(t *testing.T) { + http := &httpmock.Registry{} + client := newTestClient(http) + + repo, _ := ghrepo.FromFullName("OWNER/REPO") + + // No projects found + http.Register( + httpmock.GraphQL(`query RepositoryProjectV2List\b`), + httpmock.StringResponse(` + { "data": { "repository": { "projectsV2": { + "nodes": [], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + http.Register( + httpmock.GraphQL(`query OrganizationProjectV2List\b`), + httpmock.StringResponse(` + { "data": { "organization": { "projectsV2": { + "nodes": [], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + http.Register( + httpmock.GraphQL(`query UserProjectV2List\b`), + httpmock.StringResponse(` + { "data": { "viewer": { "projectsV2": { + "nodes": [], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + + _, err := ProjectNamesToPaths(client, repo, []string{"TriageV2"}, gh.ProjectsV1Unsupported) + require.Equal(t, err, fmt.Errorf("'TriageV2' not found")) + }) } func Test_RepoResolveMetadataIDs(t *testing.T) { diff --git a/git/client.go b/git/client.go index fe2819cf0..5f547c99c 100644 --- a/git/client.go +++ b/git/client.go @@ -518,15 +518,56 @@ func (r RemoteTrackingRef) String() string { // ParseRemoteTrackingRef parses a string of the form "refs/remotes//" into // a RemoteTrackingBranch struct. If the string does not match this format, an error is returned. +// +// For now, we assume that refnames are of the format "/", where +// the remote is a single path component, and branch may have many path components e.g. +// "origin/my/branch" is valid as: {Remote: "origin", Branch: "my/branch"} +// but "my/origin/branch" would parse incorrectly as: {Remote: "my", Branch: "origin/branch"} +// I don't believe there is a way to fix this without providing the list of remotes to this function. +// +// It becomes particularly confusing if you have something like: +// +// ``` +// [remote "foo"] +// url = https://github.com/williammartin/test-repo.git +// fetch = +refs/heads/*:refs/remotes/foo/* +// [remote "foo/bar"] +// url = https://github.com/williammartin/test-repo.git +// fetch = +refs/heads/*:refs/remotes/foo/bar/* +// [branch "bar/baz"] +// remote = foo +// merge = refs/heads/bar/baz +// [branch "baz"] +// remote = foo/bar +// merge = refs/heads/baz +// ``` +// +// These @{push} refs would resolve identically: +// +// ``` +// ➜ git rev-parse --symbolic-full-name baz@{push} +// refs/remotes/foo/bar/baz + +// ➜ git rev-parse --symbolic-full-name bar/baz@{push} +// refs/remotes/foo/bar/baz +// ``` +// +// When using this ref, git assumes it means `remote: foo` `branch: bar/baz`. func ParseRemoteTrackingRef(s string) (RemoteTrackingRef, error) { - parts := strings.Split(s, "/") - if len(parts) != 4 || parts[0] != "refs" || parts[1] != "remotes" { + prefix := "refs/remotes/" + if !strings.HasPrefix(s, prefix) { + return RemoteTrackingRef{}, fmt.Errorf("remote tracking branch must have format refs/remotes// but was: %s", s) + } + + refName := strings.TrimPrefix(s, prefix) + refNameParts := strings.SplitN(refName, "/", 2) + if len(refNameParts) != 2 { return RemoteTrackingRef{}, fmt.Errorf("remote tracking branch must have format refs/remotes// but was: %s", s) } return RemoteTrackingRef{ - Remote: parts[2], - Branch: parts[3], + Remote: refNameParts[0], + Branch: refNameParts[1], }, nil } diff --git a/git/client_test.go b/git/client_test.go index 3d7560228..f59b26077 100644 --- a/git/client_test.go +++ b/git/client_test.go @@ -1151,13 +1151,38 @@ func TestRemoteTrackingRef(t *testing.T) { wantError error }{ { - name: "valid remote tracking ref", + name: "valid remote tracking ref without slash in branch name", remoteTrackingRef: "refs/remotes/origin/branchName", wantRemoteTrackingRef: RemoteTrackingRef{ Remote: "origin", Branch: "branchName", }, }, + { + name: "valid remote tracking ref with slash in branch name", + remoteTrackingRef: "refs/remotes/origin/branch/name", + wantRemoteTrackingRef: RemoteTrackingRef{ + Remote: "origin", + Branch: "branch/name", + }, + }, + // TODO: Uncomment when we support slashes in remote names + // { + // name: "valid remote tracking ref with slash in remote name", + // remoteTrackingRef: "refs/remotes/my/origin/branchName", + // wantRemoteTrackingRef: RemoteTrackingRef{ + // Remote: "my/origin", + // Branch: "branchName", + // }, + // }, + // { + // name: "valid remote tracking ref with slash in remote name and branch name", + // remoteTrackingRef: "refs/remotes/my/origin/branch/name", + // wantRemoteTrackingRef: RemoteTrackingRef{ + // Remote: "my/origin", + // Branch: "branch/name", + // }, + // }, { name: "incorrect parts", remoteTrackingRef: "refs/remotes/origin", diff --git a/internal/config/config.go b/internal/config/config.go index c78dac15c..003a0ca17 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -18,6 +18,7 @@ import ( // they are defined here to avoid `cli/cli` being changed unexpectedly. const ( accessibleColorsKey = "accessible_colors" // used by cli/go-gh to enable the use of customizable, accessible 4-bit colors. + accessiblePrompterKey = "accessible_prompter" aliasesKey = "aliases" browserKey = "browser" // used by cli/go-gh to open URLs in web browsers colorLabelsKey = "color_labels" @@ -29,6 +30,7 @@ const ( pagerKey = "pager" promptKey = "prompt" preferEditorPromptKey = "prefer_editor_prompt" + spinnerKey = "spinner" userKey = "user" usersKey = "users" versionKey = "version" @@ -117,6 +119,11 @@ func (c *cfg) AccessibleColors(hostname string) gh.ConfigEntry { return c.GetOrDefault(hostname, accessibleColorsKey).Unwrap() } +func (c *cfg) AccessiblePrompter(hostname string) gh.ConfigEntry { + // Intentionally panic if there is no user provided value or default value (which would be a programmer error) + return c.GetOrDefault(hostname, accessiblePrompterKey).Unwrap() +} + func (c *cfg) Browser(hostname string) gh.ConfigEntry { // Intentionally panic if there is no user provided value or default value (which would be a programmer error) return c.GetOrDefault(hostname, browserKey).Unwrap() @@ -157,6 +164,11 @@ func (c *cfg) PreferEditorPrompt(hostname string) gh.ConfigEntry { return c.GetOrDefault(hostname, preferEditorPromptKey).Unwrap() } +func (c *cfg) Spinner(hostname string) gh.ConfigEntry { + // Intentionally panic if there is no user provided value or default value (which would be a programmer error) + return c.GetOrDefault(hostname, spinnerKey).Unwrap() +} + func (c *cfg) Version() o.Option[string] { return c.get("", versionKey) } @@ -550,6 +562,10 @@ browser: color_labels: disabled # Whether customizable, 4-bit accessible colors should be used. Supported values: enabled, disabled accessible_colors: disabled +# Whether an accessible prompter should be used. Supported values: enabled, disabled +accessible_prompter: disabled +# Whether to use a animated spinner as a progress indicator. If disabled, a textual progress indicator is used instead. Supported values: enabled, disabled +spinner: enabled ` type ConfigOption struct { @@ -638,6 +654,24 @@ var Options = []ConfigOption{ return c.AccessibleColors(hostname).Value }, }, + { + Key: accessiblePrompterKey, + Description: "whether an accessible prompter should be used", + DefaultValue: "disabled", + AllowedValues: []string{"enabled", "disabled"}, + CurrentValue: func(c gh.Config, hostname string) string { + return c.AccessiblePrompter(hostname).Value + }, + }, + { + Key: spinnerKey, + Description: "whether to use a animated spinner as a progress indicator", + DefaultValue: "enabled", + AllowedValues: []string{"enabled", "disabled"}, + CurrentValue: func(c gh.Config, hostname string) string { + return c.Spinner(hostname).Value + }, + }, } func HomeDirPath(subdir string) (string, error) { diff --git a/internal/config/stub.go b/internal/config/stub.go index 8b9f14290..ea60254db 100644 --- a/internal/config/stub.go +++ b/internal/config/stub.go @@ -55,6 +55,9 @@ func NewFromString(cfgStr string) *ghmock.ConfigMock { mock.AccessibleColorsFunc = func(hostname string) gh.ConfigEntry { return cfg.AccessibleColors(hostname) } + mock.AccessiblePrompterFunc = func(hostname string) gh.ConfigEntry { + return cfg.AccessiblePrompter(hostname) + } mock.BrowserFunc = func(hostname string) gh.ConfigEntry { return cfg.Browser(hostname) } @@ -79,6 +82,9 @@ func NewFromString(cfgStr string) *ghmock.ConfigMock { mock.PreferEditorPromptFunc = func(hostname string) gh.ConfigEntry { return cfg.PreferEditorPrompt(hostname) } + mock.SpinnerFunc = func(hostname string) gh.ConfigEntry { + return cfg.Spinner(hostname) + } mock.VersionFunc = func() o.Option[string] { return cfg.Version() } diff --git a/internal/featuredetection/detector_mock.go b/internal/featuredetection/detector_mock.go index 6f36dd3fc..6f760f209 100644 --- a/internal/featuredetection/detector_mock.go +++ b/internal/featuredetection/detector_mock.go @@ -1,5 +1,7 @@ package featuredetection +import "github.com/cli/cli/v2/internal/gh" + type DisabledDetectorMock struct{} func (md *DisabledDetectorMock) IssueFeatures() (IssueFeatures, error) { @@ -14,6 +16,10 @@ func (md *DisabledDetectorMock) RepositoryFeatures() (RepositoryFeatures, error) return RepositoryFeatures{}, nil } +func (md *DisabledDetectorMock) ProjectsV1() gh.ProjectsV1Support { + return gh.ProjectsV1Unsupported +} + type EnabledDetectorMock struct{} func (md *EnabledDetectorMock) IssueFeatures() (IssueFeatures, error) { @@ -27,3 +33,7 @@ func (md *EnabledDetectorMock) PullRequestFeatures() (PullRequestFeatures, error func (md *EnabledDetectorMock) RepositoryFeatures() (RepositoryFeatures, error) { return allRepositoryFeatures, nil } + +func (md *EnabledDetectorMock) ProjectsV1() gh.ProjectsV1Support { + return gh.ProjectsV1Supported +} diff --git a/internal/featuredetection/feature_detection.go b/internal/featuredetection/feature_detection.go index a9bbe25f8..fba317f58 100644 --- a/internal/featuredetection/feature_detection.go +++ b/internal/featuredetection/feature_detection.go @@ -4,6 +4,7 @@ import ( "net/http" "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/gh" "golang.org/x/sync/errgroup" ghauth "github.com/cli/go-gh/v2/pkg/auth" @@ -13,6 +14,7 @@ type Detector interface { IssueFeatures() (IssueFeatures, error) PullRequestFeatures() (PullRequestFeatures, error) RepositoryFeatures() (RepositoryFeatures, error) + ProjectsV1() gh.ProjectsV1Support } type IssueFeatures struct { @@ -199,3 +201,13 @@ func (d *detector) RepositoryFeatures() (RepositoryFeatures, error) { return features, nil } + +func (d *detector) ProjectsV1() gh.ProjectsV1Support { + // Currently, projects v1 support is entirely dependent on the host. As this is deprecated in GHES, + // we will do feature detection on whether the GHES version has support. + if ghauth.IsEnterprise(d.host) { + return gh.ProjectsV1Supported + } + + return gh.ProjectsV1Unsupported +} diff --git a/internal/featuredetection/feature_detection_test.go b/internal/featuredetection/feature_detection_test.go index 8af091c3f..f1152da2c 100644 --- a/internal/featuredetection/feature_detection_test.go +++ b/internal/featuredetection/feature_detection_test.go @@ -5,8 +5,10 @@ import ( "testing" "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/pkg/httpmock" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestIssueFeatures(t *testing.T) { @@ -366,3 +368,19 @@ func TestRepositoryFeatures(t *testing.T) { }) } } + +func TestProjectV1Support(t *testing.T) { + t.Parallel() + + t.Run("when the host is enterprise, project v1 is supported", func(t *testing.T) { + detector := detector{host: "my.ghes.com"} + isProjectV1Supported := detector.ProjectsV1() + require.Equal(t, gh.ProjectsV1Supported, isProjectV1Supported) + }) + + t.Run("when the host is not enterprise, project v1 is not supported", func(t *testing.T) { + detector := detector{host: "github.com"} + isProjectV1Supported := detector.ProjectsV1() + require.Equal(t, gh.ProjectsV1Unsupported, isProjectV1Supported) + }) +} diff --git a/internal/gh/gh.go b/internal/gh/gh.go index 8f3e3cd5b..aa90a5268 100644 --- a/internal/gh/gh.go +++ b/internal/gh/gh.go @@ -37,6 +37,8 @@ type Config interface { // AccessibleColors returns the configured accessible_colors setting, optionally scoped by host. AccessibleColors(hostname string) ConfigEntry + // AccessiblePrompter returns the configured accessible_prompter setting, optionally scoped by host. + AccessiblePrompter(hostname string) ConfigEntry // Browser returns the configured browser, optionally scoped by host. Browser(hostname string) ConfigEntry // ColorLabels returns the configured color_label setting, optionally scoped by host. @@ -53,6 +55,8 @@ type Config interface { Prompt(hostname string) ConfigEntry // PreferEditorPrompt returns the configured editor-based prompt, optionally scoped by host. PreferEditorPrompt(hostname string) ConfigEntry + // Spinner returns the configured spinner setting, optionally scoped by host. + Spinner(hostname string) ConfigEntry // Aliases provides persistent storage and modification of command aliases. Aliases() AliasConfig diff --git a/internal/gh/mock/config.go b/internal/gh/mock/config.go index 600eea5c1..9f3f80799 100644 --- a/internal/gh/mock/config.go +++ b/internal/gh/mock/config.go @@ -22,6 +22,9 @@ var _ gh.Config = &ConfigMock{} // AccessibleColorsFunc: func(hostname string) gh.ConfigEntry { // panic("mock out the AccessibleColors method") // }, +// AccessiblePrompterFunc: func(hostname string) gh.ConfigEntry { +// panic("mock out the AccessiblePrompter method") +// }, // AliasesFunc: func() gh.AliasConfig { // panic("mock out the Aliases method") // }, @@ -64,6 +67,9 @@ var _ gh.Config = &ConfigMock{} // SetFunc: func(hostname string, key string, value string) { // panic("mock out the Set method") // }, +// SpinnerFunc: func(hostname string) gh.ConfigEntry { +// panic("mock out the Spinner method") +// }, // VersionFunc: func() o.Option[string] { // panic("mock out the Version method") // }, @@ -80,6 +86,9 @@ type ConfigMock struct { // AccessibleColorsFunc mocks the AccessibleColors method. AccessibleColorsFunc func(hostname string) gh.ConfigEntry + // AccessiblePrompterFunc mocks the AccessiblePrompter method. + AccessiblePrompterFunc func(hostname string) gh.ConfigEntry + // AliasesFunc mocks the Aliases method. AliasesFunc func() gh.AliasConfig @@ -122,6 +131,9 @@ type ConfigMock struct { // SetFunc mocks the Set method. SetFunc func(hostname string, key string, value string) + // SpinnerFunc mocks the Spinner method. + SpinnerFunc func(hostname string) gh.ConfigEntry + // VersionFunc mocks the Version method. VersionFunc func() o.Option[string] @@ -135,6 +147,11 @@ type ConfigMock struct { // Hostname is the hostname argument value. Hostname string } + // AccessiblePrompter holds details about calls to the AccessiblePrompter method. + AccessiblePrompter []struct { + // Hostname is the hostname argument value. + Hostname string + } // Aliases holds details about calls to the Aliases method. Aliases []struct { } @@ -205,6 +222,11 @@ type ConfigMock struct { // Value is the value argument value. Value string } + // Spinner holds details about calls to the Spinner method. + Spinner []struct { + // Hostname is the hostname argument value. + Hostname string + } // Version holds details about calls to the Version method. Version []struct { } @@ -213,6 +235,7 @@ type ConfigMock struct { } } lockAccessibleColors sync.RWMutex + lockAccessiblePrompter sync.RWMutex lockAliases sync.RWMutex lockAuthentication sync.RWMutex lockBrowser sync.RWMutex @@ -227,6 +250,7 @@ type ConfigMock struct { lockPreferEditorPrompt sync.RWMutex lockPrompt sync.RWMutex lockSet sync.RWMutex + lockSpinner sync.RWMutex lockVersion sync.RWMutex lockWrite sync.RWMutex } @@ -263,6 +287,38 @@ func (mock *ConfigMock) AccessibleColorsCalls() []struct { return calls } +// AccessiblePrompter calls AccessiblePrompterFunc. +func (mock *ConfigMock) AccessiblePrompter(hostname string) gh.ConfigEntry { + if mock.AccessiblePrompterFunc == nil { + panic("ConfigMock.AccessiblePrompterFunc: method is nil but Config.AccessiblePrompter was just called") + } + callInfo := struct { + Hostname string + }{ + Hostname: hostname, + } + mock.lockAccessiblePrompter.Lock() + mock.calls.AccessiblePrompter = append(mock.calls.AccessiblePrompter, callInfo) + mock.lockAccessiblePrompter.Unlock() + return mock.AccessiblePrompterFunc(hostname) +} + +// AccessiblePrompterCalls gets all the calls that were made to AccessiblePrompter. +// Check the length with: +// +// len(mockedConfig.AccessiblePrompterCalls()) +func (mock *ConfigMock) AccessiblePrompterCalls() []struct { + Hostname string +} { + var calls []struct { + Hostname string + } + mock.lockAccessiblePrompter.RLock() + calls = mock.calls.AccessiblePrompter + mock.lockAccessiblePrompter.RUnlock() + return calls +} + // Aliases calls AliasesFunc. func (mock *ConfigMock) Aliases() gh.AliasConfig { if mock.AliasesFunc == nil { @@ -708,6 +764,38 @@ func (mock *ConfigMock) SetCalls() []struct { return calls } +// Spinner calls SpinnerFunc. +func (mock *ConfigMock) Spinner(hostname string) gh.ConfigEntry { + if mock.SpinnerFunc == nil { + panic("ConfigMock.SpinnerFunc: method is nil but Config.Spinner was just called") + } + callInfo := struct { + Hostname string + }{ + Hostname: hostname, + } + mock.lockSpinner.Lock() + mock.calls.Spinner = append(mock.calls.Spinner, callInfo) + mock.lockSpinner.Unlock() + return mock.SpinnerFunc(hostname) +} + +// SpinnerCalls gets all the calls that were made to Spinner. +// Check the length with: +// +// len(mockedConfig.SpinnerCalls()) +func (mock *ConfigMock) SpinnerCalls() []struct { + Hostname string +} { + var calls []struct { + Hostname string + } + mock.lockSpinner.RLock() + calls = mock.calls.Spinner + mock.lockSpinner.RUnlock() + return calls +} + // Version calls VersionFunc. func (mock *ConfigMock) Version() o.Option[string] { if mock.VersionFunc == nil { diff --git a/internal/gh/projects.go b/internal/gh/projects.go new file mode 100644 index 000000000..34acf8d7c --- /dev/null +++ b/internal/gh/projects.go @@ -0,0 +1,23 @@ +package gh + +// ProjectsV1Support provides type safety and readability around whether or not Projects v1 is supported +// by the targeted host. +// +// It is a sealed type to ensure that consumers must use the exported ProjectsV1Supported and ProjectsV1Unsupported +// variables to get an instance of the type. +type ProjectsV1Support interface { + sealed() +} + +type projectsV1Supported struct{} + +func (projectsV1Supported) sealed() {} + +type projectsV1Unsupported struct{} + +func (projectsV1Unsupported) sealed() {} + +var ( + ProjectsV1Supported ProjectsV1Support = projectsV1Supported{} + ProjectsV1Unsupported ProjectsV1Support = projectsV1Unsupported{} +) diff --git a/internal/prompter/accessible_prompter_test.go b/internal/prompter/accessible_prompter_test.go index 56096972d..619eb14f1 100644 --- a/internal/prompter/accessible_prompter_test.go +++ b/internal/prompter/accessible_prompter_test.go @@ -11,6 +11,7 @@ import ( "github.com/Netflix/go-expect" "github.com/cli/cli/v2/internal/prompter" + "github.com/cli/cli/v2/pkg/iostreams" "github.com/creack/pty" "github.com/hinshun/vt10x" "github.com/stretchr/testify/assert" @@ -33,7 +34,7 @@ import ( func TestAccessiblePrompter(t *testing.T) { t.Run("Select", func(t *testing.T) { console := newTestVirtualTerminal(t) - p := newTestAcessiblePrompter(t, console) + p := newTestAccessiblePrompter(t, console) go func() { // Wait for prompt to appear @@ -52,7 +53,7 @@ func TestAccessiblePrompter(t *testing.T) { t.Run("MultiSelect", func(t *testing.T) { console := newTestVirtualTerminal(t) - p := newTestAcessiblePrompter(t, console) + p := newTestAccessiblePrompter(t, console) go func() { // Wait for prompt to appear @@ -77,7 +78,7 @@ func TestAccessiblePrompter(t *testing.T) { t.Run("Input", func(t *testing.T) { console := newTestVirtualTerminal(t) - p := newTestAcessiblePrompter(t, console) + p := newTestAccessiblePrompter(t, console) dummyText := "12345abcdefg" go func() { @@ -97,7 +98,7 @@ func TestAccessiblePrompter(t *testing.T) { t.Run("Input - blank input returns default value", func(t *testing.T) { console := newTestVirtualTerminal(t) - p := newTestAcessiblePrompter(t, console) + p := newTestAccessiblePrompter(t, console) dummyDefaultValue := "12345abcdefg" go func() { @@ -117,7 +118,7 @@ func TestAccessiblePrompter(t *testing.T) { t.Run("Password", func(t *testing.T) { console := newTestVirtualTerminal(t) - p := newTestAcessiblePrompter(t, console) + p := newTestAccessiblePrompter(t, console) dummyPassword := "12345abcdefg" go func() { @@ -137,7 +138,7 @@ func TestAccessiblePrompter(t *testing.T) { t.Run("Confirm", func(t *testing.T) { console := newTestVirtualTerminal(t) - p := newTestAcessiblePrompter(t, console) + p := newTestAccessiblePrompter(t, console) go func() { // Wait for prompt to appear @@ -156,7 +157,7 @@ func TestAccessiblePrompter(t *testing.T) { t.Run("Confirm - blank input returns default", func(t *testing.T) { console := newTestVirtualTerminal(t) - p := newTestAcessiblePrompter(t, console) + p := newTestAccessiblePrompter(t, console) go func() { // Wait for prompt to appear @@ -175,7 +176,7 @@ func TestAccessiblePrompter(t *testing.T) { t.Run("AuthToken", func(t *testing.T) { console := newTestVirtualTerminal(t) - p := newTestAcessiblePrompter(t, console) + p := newTestAccessiblePrompter(t, console) dummyAuthToken := "12345abcdefg" go func() { @@ -195,7 +196,7 @@ func TestAccessiblePrompter(t *testing.T) { t.Run("AuthToken - blank input returns error", func(t *testing.T) { console := newTestVirtualTerminal(t) - p := newTestAcessiblePrompter(t, console) + p := newTestAccessiblePrompter(t, console) dummyAuthTokenForAfterFailure := "12345abcdefg" go func() { @@ -223,7 +224,7 @@ func TestAccessiblePrompter(t *testing.T) { t.Run("ConfirmDeletion", func(t *testing.T) { console := newTestVirtualTerminal(t) - p := newTestAcessiblePrompter(t, console) + p := newTestAccessiblePrompter(t, console) requiredValue := "test" go func() { @@ -243,7 +244,7 @@ func TestAccessiblePrompter(t *testing.T) { t.Run("ConfirmDeletion - bad input", func(t *testing.T) { console := newTestVirtualTerminal(t) - p := newTestAcessiblePrompter(t, console) + p := newTestAccessiblePrompter(t, console) requiredValue := "test" badInputValue := "garbage" @@ -272,7 +273,7 @@ func TestAccessiblePrompter(t *testing.T) { t.Run("InputHostname", func(t *testing.T) { console := newTestVirtualTerminal(t) - p := newTestAcessiblePrompter(t, console) + p := newTestAccessiblePrompter(t, console) hostname := "example.com" go func() { @@ -292,7 +293,7 @@ func TestAccessiblePrompter(t *testing.T) { t.Run("MarkdownEditor - blank allowed with blank input returns blank", func(t *testing.T) { console := newTestVirtualTerminal(t) - p := newTestAcessiblePrompter(t, console) + p := newTestAccessiblePrompter(t, console) go func() { // Wait for prompt to appear @@ -311,7 +312,7 @@ func TestAccessiblePrompter(t *testing.T) { t.Run("MarkdownEditor - blank disallowed with default value returns default value", func(t *testing.T) { console := newTestVirtualTerminal(t) - p := newTestAcessiblePrompter(t, console) + p := newTestAccessiblePrompter(t, console) defaultValue := "12345abcdefg" go func() { @@ -339,7 +340,7 @@ func TestAccessiblePrompter(t *testing.T) { t.Run("MarkdownEditor - blank disallowed no default value returns error", func(t *testing.T) { console := newTestVirtualTerminal(t) - p := newTestAcessiblePrompter(t, console) + p := newTestAccessiblePrompter(t, console) go func() { // Wait for prompt to appear @@ -419,21 +420,40 @@ func newTestVirtualTerminal(t *testing.T) *expect.Console { return console } -func newTestAcessiblePrompter(t *testing.T, console *expect.Console) prompter.Prompter { +func newTestVirtualTerminalIOStreams(t *testing.T, console *expect.Console) *iostreams.IOStreams { + t.Helper() + io := &iostreams.IOStreams{ + In: console.Tty(), + Out: console.Tty(), + ErrOut: console.Tty(), + } + io.SetStdinTTY(false) + io.SetStdoutTTY(false) + io.SetStderrTTY(false) + return io +} + +// `echo` is chosen as the editor command because it immediately returns +// a success exit code, returns an empty string, doesn't require any user input, +// and since this file is only built on Linux, it is near guaranteed to be available. +var editorCmd = "echo" + +func newTestAccessiblePrompter(t *testing.T, console *expect.Console) prompter.Prompter { t.Helper() - t.Setenv("GH_ACCESSIBLE_PROMPTER", "true") - // `echo`` is chose as the editor command because it immediately returns - // a success exit code, returns an empty string, doesn't require any user input, - // and since this file is only built on Linux, it is near guaranteed to be available. - return prompter.New("echo", console.Tty(), console.Tty(), console.Tty()) + io := newTestVirtualTerminalIOStreams(t, console) + io.SetAccessiblePrompterEnabled(true) + + return prompter.New(editorCmd, io) } func newTestSurveyPrompter(t *testing.T, console *expect.Console) prompter.Prompter { t.Helper() - t.Setenv("GH_ACCESSIBLE_PROMPTER", "false") - return prompter.New("echo", console.Tty(), console.Tty(), console.Tty()) + io := newTestVirtualTerminalIOStreams(t, console) + io.SetAccessiblePrompterEnabled(false) + + return prompter.New(editorCmd, io) } // failOnExpectError adds an observer that will fail the test in a standardised way diff --git a/internal/prompter/prompter.go b/internal/prompter/prompter.go index 6ef61cf15..2a4328366 100644 --- a/internal/prompter/prompter.go +++ b/internal/prompter/prompter.go @@ -2,13 +2,12 @@ package prompter import ( "fmt" - "os" - "slices" "strings" "github.com/AlecAivazis/survey/v2" "github.com/charmbracelet/huh" "github.com/cli/cli/v2/internal/ghinstance" + "github.com/cli/cli/v2/pkg/iostreams" "github.com/cli/cli/v2/pkg/surveyext" ghPrompter "github.com/cli/go-gh/v2/pkg/prompter" ) @@ -43,24 +42,21 @@ type Prompter interface { MarkdownEditor(prompt string, defaultValue string, blankAllowed bool) (string, error) } -func New(editorCmd string, stdin ghPrompter.FileReader, stdout ghPrompter.FileWriter, stderr ghPrompter.FileWriter) Prompter { - accessiblePrompterValue, accessiblePrompterIsSet := os.LookupEnv("GH_ACCESSIBLE_PROMPTER") - falseyValues := []string{"false", "0", "no", ""} - - if accessiblePrompterIsSet && !slices.Contains(falseyValues, accessiblePrompterValue) { +func New(editorCmd string, io *iostreams.IOStreams) Prompter { + if io.AccessiblePrompterEnabled() { return &accessiblePrompter{ - stdin: stdin, - stdout: stdout, - stderr: stderr, + stdin: io.In, + stdout: io.Out, + stderr: io.ErrOut, editorCmd: editorCmd, } } return &surveyPrompter{ - prompter: ghPrompter.New(stdin, stdout, stderr), - stdin: stdin, - stdout: stdout, - stderr: stderr, + prompter: ghPrompter.New(io.In, io.Out, io.ErrOut), + stdin: io.In, + stdout: io.Out, + stderr: io.ErrOut, editorCmd: editorCmd, } } diff --git a/pkg/cmd/config/list/list_test.go b/pkg/cmd/config/list/list_test.go index 2a1dd72ee..27260e857 100644 --- a/pkg/cmd/config/list/list_test.go +++ b/pkg/cmd/config/list/list_test.go @@ -102,6 +102,8 @@ func Test_listRun(t *testing.T) { browser=brave color_labels=disabled accessible_colors=disabled + accessible_prompter=disabled + spinner=enabled `), }, } diff --git a/pkg/cmd/factory/default.go b/pkg/cmd/factory/default.go index 7a45efeb4..52837b252 100644 --- a/pkg/cmd/factory/default.go +++ b/pkg/cmd/factory/default.go @@ -227,7 +227,7 @@ func newBrowser(f *cmdutil.Factory) browser.Browser { func newPrompter(f *cmdutil.Factory) prompter.Prompter { editor, _ := cmdutil.DetermineEditor(f.Config) io := f.IOStreams - return prompter.New(editor, io.In, io.Out, io.ErrOut) + return prompter.New(editor, io) } func configFunc() func() (gh.Config, error) { @@ -284,9 +284,23 @@ func ioStreams(f *cmdutil.Factory) *iostreams.IOStreams { io.SetNeverPrompt(true) } - ghSpinnerDisabledValue, ghSpinnerDisabledIsSet := os.LookupEnv("GH_SPINNER_DISABLED") falseyValues := []string{"false", "0", "no", ""} - if ghSpinnerDisabledIsSet && !slices.Contains(falseyValues, ghSpinnerDisabledValue) { + + accessiblePrompterValue, accessiblePrompterIsSet := os.LookupEnv("GH_ACCESSIBLE_PROMPTER") + if accessiblePrompterIsSet { + if !slices.Contains(falseyValues, accessiblePrompterValue) { + io.SetAccessiblePrompterEnabled(true) + } + } else if prompt := cfg.AccessiblePrompter(""); prompt.Value == "enabled" { + io.SetAccessiblePrompterEnabled(true) + } + + ghSpinnerDisabledValue, ghSpinnerDisabledIsSet := os.LookupEnv("GH_SPINNER_DISABLED") + if ghSpinnerDisabledIsSet { + if !slices.Contains(falseyValues, ghSpinnerDisabledValue) { + io.SetSpinnerDisabled(true) + } + } else if spinnerDisabled := cfg.Spinner(""); spinnerDisabled.Value == "disabled" { io.SetSpinnerDisabled(true) } diff --git a/pkg/cmd/factory/default_test.go b/pkg/cmd/factory/default_test.go index 5036a1dc1..d7bfe39fd 100644 --- a/pkg/cmd/factory/default_test.go +++ b/pkg/cmd/factory/default_test.go @@ -435,6 +435,7 @@ func Test_ioStreams_prompt(t *testing.T) { func Test_ioStreams_spinnerDisabled(t *testing.T) { tests := []struct { name string + config gh.Config spinnerDisabled bool env map[string]string }{ @@ -442,6 +443,16 @@ func Test_ioStreams_spinnerDisabled(t *testing.T) { name: "default config", spinnerDisabled: false, }, + { + name: "config with spinner disabled", + config: disableSpinnersConfig(), + spinnerDisabled: true, + }, + { + name: "config with spinner enabled", + config: enableSpinnersConfig(), + spinnerDisabled: false, + }, { name: "spinner disabled via GH_SPINNER_DISABLED env var = 0", env: map[string]string{"GH_SPINNER_DISABLED": "0"}, @@ -467,6 +478,18 @@ func Test_ioStreams_spinnerDisabled(t *testing.T) { env: map[string]string{"GH_SPINNER_DISABLED": "true"}, spinnerDisabled: true, }, + { + name: "config enabled but env disabled, respects env", + config: enableSpinnersConfig(), + env: map[string]string{"GH_SPINNER_DISABLED": "true"}, + spinnerDisabled: true, + }, + { + name: "config disabled but env enabled, respects env", + config: disableSpinnersConfig(), + env: map[string]string{"GH_SPINNER_DISABLED": "false"}, + spinnerDisabled: false, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -474,12 +497,87 @@ func Test_ioStreams_spinnerDisabled(t *testing.T) { t.Setenv(k, v) } f := New("1") + f.Config = func() (gh.Config, error) { + if tt.config == nil { + return config.NewBlankConfig(), nil + } else { + return tt.config, nil + } + } io := ioStreams(f) assert.Equal(t, tt.spinnerDisabled, io.GetSpinnerDisabled()) }) } } +func Test_ioStreams_accessiblePrompterEnabled(t *testing.T) { + tests := []struct { + name string + config gh.Config + accessiblePrompterEnabled bool + env map[string]string + }{ + { + name: "default config", + accessiblePrompterEnabled: false, + }, + { + name: "config with accessible prompter enabled", + config: enableAccessiblePrompterConfig(), + accessiblePrompterEnabled: true, + }, + { + name: "config with accessible prompter disabled", + config: disableAccessiblePrompterConfig(), + accessiblePrompterEnabled: false, + }, + { + name: "accessible prompter enabled via GH_ACCESSIBLE_PROMPTER env var = 1", + env: map[string]string{"GH_ACCESSIBLE_PROMPTER": "1"}, + accessiblePrompterEnabled: true, + }, + { + name: "accessible prompter enabled via GH_ACCESSIBLE_PROMPTER env var = true", + env: map[string]string{"GH_ACCESSIBLE_PROMPTER": "true"}, + accessiblePrompterEnabled: true, + }, + { + name: "accessible prompter disabled via GH_ACCESSIBLE_PROMPTER env var = 0", + env: map[string]string{"GH_ACCESSIBLE_PROMPTER": "0"}, + accessiblePrompterEnabled: false, + }, + { + name: "config disabled but env enabled, respects env", + config: disableAccessiblePrompterConfig(), + env: map[string]string{"GH_ACCESSIBLE_PROMPTER": "true"}, + accessiblePrompterEnabled: true, + }, + { + name: "config enabled but env disabled, respects env", + config: enableAccessiblePrompterConfig(), + env: map[string]string{"GH_ACCESSIBLE_PROMPTER": "false"}, + accessiblePrompterEnabled: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + for k, v := range tt.env { + t.Setenv(k, v) + } + f := New("1") + f.Config = func() (gh.Config, error) { + if tt.config == nil { + return config.NewBlankConfig(), nil + } else { + return tt.config, nil + } + } + io := ioStreams(f) + assert.Equal(t, tt.accessiblePrompterEnabled, io.AccessiblePrompterEnabled()) + }) + } +} + func Test_ioStreams_colorLabels(t *testing.T) { tests := []struct { name string @@ -664,6 +762,22 @@ func disablePromptConfig() gh.Config { return config.NewFromString("prompt: disabled") } +func enableAccessiblePrompterConfig() gh.Config { + return config.NewFromString("accessible_prompter: enabled") +} + +func disableAccessiblePrompterConfig() gh.Config { + return config.NewFromString("accessible_prompter: disabled") +} + +func disableSpinnersConfig() gh.Config { + return config.NewFromString("spinner: disabled") +} + +func enableSpinnersConfig() gh.Config { + return config.NewFromString("spinner: enabled") +} + func disableColorLabelsConfig() gh.Config { return config.NewFromString("color_labels: disabled") } diff --git a/pkg/cmd/issue/create/create.go b/pkg/cmd/issue/create/create.go index 2e3e0de51..2978a21fc 100644 --- a/pkg/cmd/issue/create/create.go +++ b/pkg/cmd/issue/create/create.go @@ -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" @@ -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 @@ -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}), } @@ -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 @@ -160,13 +173,13 @@ func createRun(opts *CreateOptions) (err error) { } tb := prShared.IssueMetadataState{ - Type: prShared.IssueMetadata, - Assignees: assignees, - Labels: opts.Labels, - Projects: opts.Projects, - Milestones: milestones, - Title: opts.Title, - Body: opts.Body, + Type: prShared.IssueMetadata, + Assignees: assignees, + Labels: opts.Labels, + ProjectTitles: opts.Projects, + Milestones: milestones, + Title: opts.Title, + Body: opts.Body, } if opts.RecoverFile != "" { @@ -182,7 +195,7 @@ func createRun(opts *CreateOptions) (err error) { if opts.WebMode { var openURL string if opts.Title != "" || opts.Body != "" || tb.HasMetadata() { - openURL, err = generatePreviewURL(apiClient, baseRepo, tb) + openURL, err = generatePreviewURL(apiClient, baseRepo, tb, projectsV1Support) if err != nil { return } @@ -260,7 +273,7 @@ func createRun(opts *CreateOptions) (err error) { } } - openURL, err = generatePreviewURL(apiClient, baseRepo, tb) + openURL, err = generatePreviewURL(apiClient, baseRepo, tb, projectsV1Support) if err != nil { return } @@ -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) if err != nil { return } @@ -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 } @@ -354,7 +367,7 @@ func createRun(opts *CreateOptions) (err error) { return } -func generatePreviewURL(apiClient *api.Client, baseRepo ghrepo.Interface, tb prShared.IssueMetadataState) (string, error) { +func generatePreviewURL(apiClient *api.Client, baseRepo ghrepo.Interface, tb prShared.IssueMetadataState, projectsV1Support gh.ProjectsV1Support) (string, error) { openURL := ghrepo.GenerateRepoURL(baseRepo, "issues/new") - return prShared.WithPrAndIssueQueryParams(apiClient, baseRepo, openURL, tb) + return prShared.WithPrAndIssueQueryParams(apiClient, baseRepo, openURL, tb, projectsV1Support) } diff --git a/pkg/cmd/issue/create/create_test.go b/pkg/cmd/issue/create/create_test.go index 8e49700a0..1211c0c1d 100644 --- a/pkg/cmd/issue/create/create_test.go +++ b/pkg/cmd/issue/create/create_test.go @@ -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" @@ -473,6 +474,7 @@ func Test_createRun(t *testing.T) { opts.BaseRepo = func() (ghrepo.Interface, error) { return ghrepo.New("OWNER", "REPO"), nil } + opts.Detector = &fd.EnabledDetectorMock{} browser := &browser.Stub{} opts.Browser = browser @@ -521,6 +523,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) }) @@ -1026,3 +1029,146 @@ 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, ""), + ) + + _, 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) + }) + }) + + t.Run("web mode", 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.Register( + // ( is required to avoid matching projectsV2 + httpmock.GraphQL(`projects\(`), + // Simulate a GraphQL error to early exit the test. + httpmock.StatusStringResponse(500, ""), + ) + + _, 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{}, + WebMode: true, + // 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{} + // ( 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{}, + WebMode: true, + // Required to force a lookup of projects + Projects: []string{"Project"}, + }) + + // Verify that our request contained projectCards + reg.Verify(t) + }) + }) +} diff --git a/pkg/cmd/issue/edit/edit.go b/pkg/cmd/issue/edit/edit.go index accea8add..8386cbcfa 100644 --- a/pkg/cmd/issue/edit/edit.go +++ b/pkg/cmd/issue/edit/edit.go @@ -5,9 +5,12 @@ import ( "net/http" "sort" "sync" + "time" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" + 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" shared "github.com/cli/cli/v2/pkg/cmd/issue/shared" @@ -22,6 +25,7 @@ type EditOptions struct { IO *iostreams.IOStreams BaseRepo func() (ghrepo.Interface, error) Prompter prShared.EditPrompter + Detector fd.Detector DetermineEditor func() (string, error) FieldsToEditSurvey func(prShared.EditPrompter, *prShared.Editable) error @@ -201,7 +205,18 @@ func editRun(opts *EditOptions) error { lookupFields = append(lookupFields, "labels") } if editable.Projects.Edited { - lookupFields = append(lookupFields, "projectCards") + // TODO projectsV1Deprecation + // Remove this section as we should no longer add projectCards + if opts.Detector == nil { + cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24) + opts.Detector = fd.NewDetector(cachedClient, baseRepo.RepoHost()) + } + + projectsV1Support := opts.Detector.ProjectsV1() + if projectsV1Support == gh.ProjectsV1Supported { + lookupFields = append(lookupFields, "projectCards") + } + lookupFields = append(lookupFields, "projectItems") } if editable.Milestone.Edited { diff --git a/pkg/cmd/issue/edit/edit_test.go b/pkg/cmd/issue/edit/edit_test.go index a9d43c3ec..c9aa4c409 100644 --- a/pkg/cmd/issue/edit/edit_test.go +++ b/pkg/cmd/issue/edit/edit_test.go @@ -10,7 +10,9 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" + fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/run" prShared "github.com/cli/cli/v2/pkg/cmd/pr/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" @@ -788,3 +790,88 @@ func mockProjectV2ItemUpdate(t *testing.T, reg *httpmock.Registry) { func(inputs map[string]interface{}) {}), ) } + +// TODO projectsV1Deprecation +// Remove this test. +func TestProjectsV1Deprecation(t *testing.T) { + t.Run("when projects v1 is supported, is included in query", func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + + reg := &httpmock.Registry{} + reg.Register( + httpmock.GraphQL(`projectCards`), + // Simulate a GraphQL error to early exit the test. + httpmock.StatusStringResponse(500, ""), + ) + + _, 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. + _ = editRun(&EditOptions{ + 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{}, + + IssueNumbers: []int{123}, + Editable: prShared.Editable{ + Projects: prShared.EditableProjects{ + EditableSlice: prShared.EditableSlice{ + Add: []string{"Test Project"}, + Edited: true, + }, + }, + }, + }) + + // Verify that our request contained projectCards + reg.Verify(t) + }) + + t.Run("when projects v1 is not supported, is not included in query", func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + + reg := &httpmock.Registry{} + reg.Exclude(t, httpmock.GraphQL(`projectCards`)) + + reg.Register( + httpmock.GraphQL(`.*`), + // Simulate a GraphQL error to early exit the test. + httpmock.StatusStringResponse(500, ""), + ) + + _, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + // Ignore the error because we're not really interested in it. + _ = editRun(&EditOptions{ + 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{}, + + IssueNumbers: []int{123}, + Editable: prShared.Editable{ + Projects: prShared.EditableProjects{ + EditableSlice: prShared.EditableSlice{ + Add: []string{"Test Project"}, + Edited: true, + }, + }, + }, + }) + + // Verify that our request contained projectCards + reg.Verify(t) + }) +} diff --git a/pkg/cmd/issue/view/view.go b/pkg/cmd/issue/view/view.go index f7838429b..a9e25513b 100644 --- a/pkg/cmd/issue/view/view.go +++ b/pkg/cmd/issue/view/view.go @@ -12,6 +12,8 @@ import ( "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" "github.com/cli/cli/v2/pkg/cmd/issue/shared" @@ -29,6 +31,7 @@ type ViewOptions struct { IO *iostreams.IOStreams BaseRepo func() (ghrepo.Interface, error) Browser browser.Browser + Detector fd.Detector IssueNumber int WebMode bool @@ -89,7 +92,7 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman var defaultFields = []string{ "number", "url", "state", "createdAt", "title", "body", "author", "milestone", - "assignees", "labels", "projectCards", "reactionGroups", "lastComment", "stateReason", + "assignees", "labels", "reactionGroups", "lastComment", "stateReason", } func viewRun(opts *ViewOptions) error { @@ -114,6 +117,18 @@ func viewRun(opts *ViewOptions) error { lookupFields.Add("comments") lookupFields.Remove("lastComment") } + + // TODO projectsV1Deprecation + // Remove this section as we should no longer add projectCards + if opts.Detector == nil { + cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24) + opts.Detector = fd.NewDetector(cachedClient, baseRepo.RepoHost()) + } + + projectsV1Support := opts.Detector.ProjectsV1() + if projectsV1Support == gh.ProjectsV1Supported { + lookupFields.Add("projectCards") + } } opts.IO.DetectTerminalTheme() diff --git a/pkg/cmd/issue/view/view_test.go b/pkg/cmd/issue/view/view_test.go index 2dd963687..391a288fb 100644 --- a/pkg/cmd/issue/view/view_test.go +++ b/pkg/cmd/issue/view/view_test.go @@ -10,6 +10,7 @@ import ( "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/run" @@ -496,3 +497,66 @@ func TestIssueView_nontty_Comments(t *testing.T) { }) } } + +// TODO projectsV1Deprecation +// Remove this test. +func TestProjectsV1Deprecation(t *testing.T) { + t.Run("when projects v1 is supported, is included in query", func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + + reg := &httpmock.Registry{} + reg.Register( + httpmock.GraphQL(`projectCards`), + // Simulate a GraphQL error to early exit the test. + httpmock.StatusStringResponse(500, ""), + ) + + _, 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. + _ = viewRun(&ViewOptions{ + 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{}, + IssueNumber: 123, + }) + + // Verify that our request contained projectCards + reg.Verify(t) + }) + + t.Run("when projects v1 is not supported, is not included in query", func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + + reg := &httpmock.Registry{} + reg.Exclude(t, httpmock.GraphQL(`projectCards`)) + + _, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + // Ignore the error because we're not really interested in it. + _ = viewRun(&ViewOptions{ + 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{}, + IssueNumber: 123, + }) + + // Verify that our request contained projectCards + reg.Verify(t) + }) +} diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index eda7a3ce7..7f960bce4 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -440,7 +440,8 @@ func createRun(opts *CreateOptions) error { if err != nil { return err } - return submitPR(*opts, *ctx, *state) + // TODO wm: revisit project support + return submitPR(*opts, *ctx, *state, gh.ProjectsV1Supported) } if opts.RecoverFile != "" { @@ -536,7 +537,8 @@ func createRun(opts *CreateOptions) error { Repo: ctx.PRRefs.BaseRepo(), State: state, } - err = shared.MetadataSurvey(opts.Prompter, opts.IO, ctx.PRRefs.BaseRepo(), fetcher, state) + // TODO wm: revisit project support + err = shared.MetadataSurvey(opts.Prompter, opts.IO, ctx.PRRefs.BaseRepo(), fetcher, state, gh.ProjectsV1Supported) if err != nil { return err } @@ -565,11 +567,13 @@ func createRun(opts *CreateOptions) error { if action == shared.SubmitDraftAction { state.Draft = true - return submitPR(*opts, *ctx, *state) + // TODO wm: revisit project support + return submitPR(*opts, *ctx, *state, gh.ProjectsV1Supported) } if action == shared.SubmitAction { - return submitPR(*opts, *ctx, *state) + // TODO wm: revisit project support + return submitPR(*opts, *ctx, *state, gh.ProjectsV1Supported) } err = errors.New("expected to cancel, preview, or submit") @@ -621,13 +625,13 @@ func NewIssueState(ctx CreateContext, opts CreateOptions) (*shared.IssueMetadata } state := &shared.IssueMetadataState{ - Type: shared.PRMetadata, - Reviewers: opts.Reviewers, - Assignees: assignees, - Labels: opts.Labels, - Projects: opts.Projects, - Milestones: milestoneTitles, - Draft: opts.IsDraft, + Type: shared.PRMetadata, + Reviewers: opts.Reviewers, + Assignees: assignees, + Labels: opts.Labels, + ProjectTitles: opts.Projects, + Milestones: milestoneTitles, + Draft: opts.IsDraft, } if opts.FillVerbose || opts.Autofill || opts.FillFirst || !opts.TitleProvided || !opts.BodyProvided { @@ -966,7 +970,7 @@ func getRemotes(opts *CreateOptions) (ghContext.Remotes, error) { return remotes, nil } -func submitPR(opts CreateOptions, ctx CreateContext, state shared.IssueMetadataState) error { +func submitPR(opts CreateOptions, ctx CreateContext, state shared.IssueMetadataState, projectV1Support gh.ProjectsV1Support) error { client := ctx.Client params := map[string]interface{}{ @@ -982,7 +986,7 @@ func submitPR(opts CreateOptions, ctx CreateContext, state shared.IssueMetadataS return errors.New("pull request title must not be blank") } - err := shared.AddMetadataToIssueParams(client, ctx.PRRefs.BaseRepo(), params, &state) + err := shared.AddMetadataToIssueParams(client, ctx.PRRefs.BaseRepo(), params, &state, projectV1Support) if err != nil { return err } @@ -1028,8 +1032,8 @@ func renderPullRequestPlain(w io.Writer, params map[string]interface{}, state *s if len(state.Milestones) != 0 { fmt.Fprintf(w, "milestones:\t%v\n", strings.Join(state.Milestones, ", ")) } - if len(state.Projects) != 0 { - fmt.Fprintf(w, "projects:\t%v\n", strings.Join(state.Projects, ", ")) + if len(state.ProjectTitles) != 0 { + fmt.Fprintf(w, "projects:\t%v\n", strings.Join(state.ProjectTitles, ", ")) } fmt.Fprintf(w, "maintainerCanModify:\t%t\n", params["maintainerCanModify"]) fmt.Fprint(w, "body:\n") @@ -1060,8 +1064,8 @@ func renderPullRequestTTY(io *iostreams.IOStreams, params map[string]interface{} if len(state.Milestones) != 0 { fmt.Fprintf(out, "%s: %s\n", cs.Bold("Milestones"), strings.Join(state.Milestones, ", ")) } - if len(state.Projects) != 0 { - fmt.Fprintf(out, "%s: %s\n", cs.Bold("Projects"), strings.Join(state.Projects, ", ")) + if len(state.ProjectTitles) != 0 { + fmt.Fprintf(out, "%s: %s\n", cs.Bold("Projects"), strings.Join(state.ProjectTitles, ", ")) } fmt.Fprintf(out, "%s: %t\n", cs.Bold("MaintainerCanModify"), params["maintainerCanModify"]) @@ -1217,7 +1221,8 @@ func generateCompareURL(ctx CreateContext, state shared.IssueMetadataState) (str ctx.PRRefs.BaseRepo(), "compare/%s...%s?expand=1", url.PathEscape(ctx.PRRefs.BaseRef()), url.PathEscape(ctx.PRRefs.QualifiedHeadRef())) - url, err := shared.WithPrAndIssueQueryParams(ctx.Client, ctx.PRRefs.BaseRepo(), u, state) + // TODO wm: revisit project support + url, err := shared.WithPrAndIssueQueryParams(ctx.Client, ctx.PRRefs.BaseRepo(), u, state, gh.ProjectsV1Supported) if err != nil { return "", err } diff --git a/pkg/cmd/pr/shared/editable.go b/pkg/cmd/pr/shared/editable.go index cec3bfe8c..0bebb999a 100644 --- a/pkg/cmd/pr/shared/editable.go +++ b/pkg/cmd/pr/shared/editable.go @@ -381,7 +381,8 @@ func FetchOptions(client *api.Client, repo ghrepo.Interface, editable *Editable) Reviewers: editable.Reviewers.Edited, Assignees: editable.Assignees.Edited, Labels: editable.Labels.Edited, - Projects: editable.Projects.Edited, + ProjectsV1: editable.Projects.Edited, + ProjectsV2: editable.Projects.Edited, Milestones: editable.Milestone.Edited, } metadata, err := api.RepoMetadata(client, repo, input) diff --git a/pkg/cmd/pr/shared/find_refs_resolution.go b/pkg/cmd/pr/shared/find_refs_resolution.go index 833075af8..e4e51bab8 100644 --- a/pkg/cmd/pr/shared/find_refs_resolution.go +++ b/pkg/cmd/pr/shared/find_refs_resolution.go @@ -333,12 +333,12 @@ func tryDetermineDefaultPushTarget(gitClient GitConfigClient, localBranchName st } // We assume the PR's branch name is the same as whatever was provided, unless the user has specified - // push.default = upstream or tracking, then we use the branch name from the merge ref. + // push.default = upstream or tracking, then we use the branch name from the merge ref if it exists. Otherwise, we fall back to the local branch name remoteBranch := localBranchName if pushDefault == git.PushDefaultUpstream || pushDefault == git.PushDefaultTracking { - remoteBranch = strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/") - if remoteBranch == "" { - return defaultPushTarget{}, fmt.Errorf("could not determine remote branch name") + mergeRef := strings.TrimPrefix(branchConfig.MergeRef, "refs/heads/") + if mergeRef != "" { + remoteBranch = mergeRef } } diff --git a/pkg/cmd/pr/shared/find_refs_resolution_test.go b/pkg/cmd/pr/shared/find_refs_resolution_test.go index 8cbb62146..d2393bf10 100644 --- a/pkg/cmd/pr/shared/find_refs_resolution_test.go +++ b/pkg/cmd/pr/shared/find_refs_resolution_test.go @@ -462,7 +462,7 @@ func TestTryDetermineDefaultPRHead(t *testing.T) { }) } - t.Run("but if the merge ref is empty, error", func(t *testing.T) { + t.Run("but if the merge ref is empty, use the provided branch name", func(t *testing.T) { t.Parallel() repoResolvedFromPushRemoteClient := stubGitConfigClient{ @@ -474,12 +474,14 @@ func TestTryDetermineDefaultPRHead(t *testing.T) { pushDefaultFn: stubPushDefault(git.PushDefaultUpstream, nil), } - _, err := TryDetermineDefaultPRHead( + defaultPRHead, err := TryDetermineDefaultPRHead( repoResolvedFromPushRemoteClient, stubRemoteToRepoResolver(ghContext.Remotes{&baseRemote, &forkRemote}, nil), "feature-branch", ) - require.Error(t, err) + require.NoError(t, err) + + require.Equal(t, "feature-branch", defaultPRHead.BranchName) }) }) diff --git a/pkg/cmd/pr/shared/params.go b/pkg/cmd/pr/shared/params.go index 128c51068..4f36a80aa 100644 --- a/pkg/cmd/pr/shared/params.go +++ b/pkg/cmd/pr/shared/params.go @@ -6,12 +6,13 @@ import ( "strings" "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/search" "github.com/google/shlex" ) -func WithPrAndIssueQueryParams(client *api.Client, baseRepo ghrepo.Interface, baseURL string, state IssueMetadataState) (string, error) { +func WithPrAndIssueQueryParams(client *api.Client, baseRepo ghrepo.Interface, baseURL string, state IssueMetadataState, projectsV1Support gh.ProjectsV1Support) (string, error) { u, err := url.Parse(baseURL) if err != nil { return "", err @@ -34,8 +35,8 @@ func WithPrAndIssueQueryParams(client *api.Client, baseRepo ghrepo.Interface, ba if len(state.Labels) > 0 { q.Set("labels", strings.Join(state.Labels, ",")) } - if len(state.Projects) > 0 { - projectPaths, err := api.ProjectNamesToPaths(client, baseRepo, state.Projects) + if len(state.ProjectTitles) > 0 { + projectPaths, err := api.ProjectNamesToPaths(client, baseRepo, state.ProjectTitles, projectsV1Support) if err != nil { return "", fmt.Errorf("could not add to project: %w", err) } @@ -56,7 +57,7 @@ func ValidURL(urlStr string) bool { // Ensure that tb.MetadataResult object exists and contains enough pre-fetched API data to be able // to resolve all object listed in tb to GraphQL IDs. -func fillMetadata(client *api.Client, baseRepo ghrepo.Interface, tb *IssueMetadataState) error { +func fillMetadata(client *api.Client, baseRepo ghrepo.Interface, tb *IssueMetadataState, projectV1Support gh.ProjectsV1Support) error { resolveInput := api.RepoResolveInput{} if len(tb.Assignees) > 0 && (tb.MetadataResult == nil || len(tb.MetadataResult.AssignableUsers) == 0) { @@ -71,8 +72,12 @@ func fillMetadata(client *api.Client, baseRepo ghrepo.Interface, tb *IssueMetada resolveInput.Labels = tb.Labels } - if len(tb.Projects) > 0 && (tb.MetadataResult == nil || len(tb.MetadataResult.Projects) == 0) { - resolveInput.Projects = tb.Projects + if len(tb.ProjectTitles) > 0 && (tb.MetadataResult == nil || len(tb.MetadataResult.Projects) == 0) { + if projectV1Support == gh.ProjectsV1Supported { + resolveInput.ProjectsV1 = true + } + + resolveInput.ProjectsV2 = true } if len(tb.Milestones) > 0 && (tb.MetadataResult == nil || len(tb.MetadataResult.Milestones) == 0) { @@ -93,12 +98,12 @@ func fillMetadata(client *api.Client, baseRepo ghrepo.Interface, tb *IssueMetada return nil } -func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, params map[string]interface{}, tb *IssueMetadataState) error { +func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, params map[string]interface{}, tb *IssueMetadataState, projectV1Support gh.ProjectsV1Support) error { if !tb.HasMetadata() { return nil } - if err := fillMetadata(client, baseRepo, tb); err != nil { + if err := fillMetadata(client, baseRepo, tb, projectV1Support); err != nil { return err } @@ -114,7 +119,7 @@ func AddMetadataToIssueParams(client *api.Client, baseRepo ghrepo.Interface, par } params["labelIds"] = labelIDs - projectIDs, projectV2IDs, err := tb.MetadataResult.ProjectsToIDs(tb.Projects) + projectIDs, projectV2IDs, err := tb.MetadataResult.ProjectsToIDs(tb.ProjectTitles) if err != nil { return fmt.Errorf("could not add to project: %w", err) } diff --git a/pkg/cmd/pr/shared/params_test.go b/pkg/cmd/pr/shared/params_test.go index 5f5e674cc..15f00ca4f 100644 --- a/pkg/cmd/pr/shared/params_test.go +++ b/pkg/cmd/pr/shared/params_test.go @@ -2,13 +2,16 @@ package shared import ( "net/http" + "net/url" "reflect" "testing" "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/gh" "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 Test_listURLWithQuery(t *testing.T) { @@ -265,7 +268,7 @@ func Test_WithPrAndIssueQueryParams(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := WithPrAndIssueQueryParams(nil, nil, tt.args.baseURL, tt.args.state) + got, err := WithPrAndIssueQueryParams(nil, nil, tt.args.baseURL, tt.args.state, gh.ProjectsV1Supported) if (err != nil) != tt.wantErr { t.Errorf("WithPrAndIssueQueryParams() error = %v, wantErr %v", err, tt.wantErr) return @@ -276,3 +279,144 @@ func Test_WithPrAndIssueQueryParams(t *testing.T) { }) } } + +// TODO projectsV1Deprecation +// Remove this test. +func TestWithPrAndIssueQueryParamsProjectsV1Deprecation(t *testing.T) { + t.Run("when projectsV1 is supported, requests them", func(t *testing.T) { + reg := &httpmock.Registry{} + client := api.NewClientFromHTTP(&http.Client{ + Transport: reg, + }) + + repo, _ := ghrepo.FromFullName("OWNER/REPO") + + reg.Register( + httpmock.GraphQL(`query RepositoryProjectList\b`), + httpmock.StringResponse(` + { "data": { "repository": { "projects": { + "nodes": [], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query OrganizationProjectList\b`), + httpmock.StringResponse(` + { "data": { "organization": { "projects": { + "nodes": [ + { "name": "Triage", "id": "TRIAGEID", "resourcePath": "/orgs/ORG/projects/1" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query RepositoryProjectV2List\b`), + httpmock.StringResponse(` + { "data": { "repository": { "projectsV2": { + "nodes": [], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query OrganizationProjectV2List\b`), + httpmock.StringResponse(` + { "data": { "organization": { "projectsV2": { + "nodes": [], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query UserProjectV2List\b`), + httpmock.StringResponse(` + { "data": { "viewer": { "projectsV2": { + "nodes": [], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + + u, err := WithPrAndIssueQueryParams( + client, + repo, + "http://example.com/hey", + IssueMetadataState{ + ProjectTitles: []string{"Triage"}, + }, + gh.ProjectsV1Supported, + ) + require.NoError(t, err) + + url, err := url.Parse(u) + require.NoError(t, err) + + require.Equal( + t, + url.Query().Get("projects"), + "ORG/1", + ) + }) + + t.Run("when projectsV1 is not supported, does not request them", func(t *testing.T) { + reg := &httpmock.Registry{} + client := api.NewClientFromHTTP(&http.Client{ + Transport: reg, + }) + + repo, _ := ghrepo.FromFullName("OWNER/REPO") + + reg.Exclude( + t, + httpmock.GraphQL(`query RepositoryProjectList\b`), + ) + reg.Exclude( + t, + httpmock.GraphQL(`query OrganizationProjectList\b`), + ) + + reg.Register( + httpmock.GraphQL(`query RepositoryProjectV2List\b`), + httpmock.StringResponse(` + { "data": { "repository": { "projectsV2": { + "nodes": [], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query OrganizationProjectV2List\b`), + httpmock.StringResponse(` + { "data": { "organization": { "projectsV2": { + "nodes": [ + { "title": "TriageV2", "id": "TRIAGEV2ID", "resourcePath": "/orgs/ORG/projects/2" } + ], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + reg.Register( + httpmock.GraphQL(`query UserProjectV2List\b`), + httpmock.StringResponse(` + { "data": { "viewer": { "projectsV2": { + "nodes": [], + "pageInfo": { "hasNextPage": false } + } } } } + `)) + + u, err := WithPrAndIssueQueryParams( + client, + repo, + "http://example.com/hey", + IssueMetadataState{ + ProjectTitles: []string{"TriageV2"}, + }, + gh.ProjectsV1Unsupported, + ) + require.NoError(t, err) + + url, err := url.Parse(u) + require.NoError(t, err) + + require.Equal( + t, + url.Query().Get("projects"), + "ORG/2", + ) + }) +} diff --git a/pkg/cmd/pr/shared/state.go b/pkg/cmd/pr/shared/state.go index 143021cb6..7e7da436d 100644 --- a/pkg/cmd/pr/shared/state.go +++ b/pkg/cmd/pr/shared/state.go @@ -25,12 +25,12 @@ type IssueMetadataState struct { Template string - Metadata []string - Reviewers []string - Assignees []string - Labels []string - Projects []string - Milestones []string + Metadata []string + Reviewers []string + Assignees []string + Labels []string + ProjectTitles []string + Milestones []string MetadataResult *api.RepoMetadataResult @@ -49,7 +49,7 @@ func (tb *IssueMetadataState) HasMetadata() bool { return len(tb.Reviewers) > 0 || len(tb.Assignees) > 0 || len(tb.Labels) > 0 || - len(tb.Projects) > 0 || + len(tb.ProjectTitles) > 0 || len(tb.Milestones) > 0 } diff --git a/pkg/cmd/pr/shared/survey.go b/pkg/cmd/pr/shared/survey.go index ce38535d9..bf4476ca1 100644 --- a/pkg/cmd/pr/shared/survey.go +++ b/pkg/cmd/pr/shared/survey.go @@ -151,7 +151,7 @@ type RepoMetadataFetcher interface { RepoMetadataFetch(api.RepoMetadataInput) (*api.RepoMetadataResult, error) } -func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface, fetcher RepoMetadataFetcher, state *IssueMetadataState) error { +func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface, fetcher RepoMetadataFetcher, state *IssueMetadataState, projectsV1Support gh.ProjectsV1Support) error { isChosen := func(m string) bool { for _, c := range state.Metadata { if m == c { @@ -181,7 +181,8 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface Reviewers: isChosen("Reviewers"), Assignees: isChosen("Assignees"), Labels: isChosen("Labels"), - Projects: isChosen("Projects"), + ProjectsV1: isChosen("Projects") && projectsV1Support == gh.ProjectsV1Supported, + ProjectsV2: isChosen("Projects"), Milestones: isChosen("Milestone"), } metadataResult, err := fetcher.RepoMetadataFetch(metadataInput) @@ -267,7 +268,7 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface } if isChosen("Projects") { if len(projects) > 0 { - selected, err := p.MultiSelect("Projects", state.Projects, projects) + selected, err := p.MultiSelect("Projects", state.ProjectTitles, projects) if err != nil { return err } @@ -316,7 +317,7 @@ func MetadataSurvey(p Prompt, io *iostreams.IOStreams, baseRepo ghrepo.Interface state.Labels = values.Labels } if isChosen("Projects") { - state.Projects = values.Projects + state.ProjectTitles = values.Projects } if isChosen("Milestone") { if values.Milestone != "" && values.Milestone != noMilestone { diff --git a/pkg/cmd/pr/shared/survey_test.go b/pkg/cmd/pr/shared/survey_test.go index d74696460..6895b52ac 100644 --- a/pkg/cmd/pr/shared/survey_test.go +++ b/pkg/cmd/pr/shared/survey_test.go @@ -1,13 +1,16 @@ package shared import ( + "errors" "testing" "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/pkg/iostreams" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) type metadataFetcher struct { @@ -68,7 +71,7 @@ func TestMetadataSurvey_selectAll(t *testing.T) { Assignees: []string{"hubot"}, Type: PRMetadata, } - err := MetadataSurvey(pm, ios, repo, fetcher, state) + err := MetadataSurvey(pm, ios, repo, fetcher, state, gh.ProjectsV1Supported) assert.NoError(t, err) assert.Equal(t, "", stdout.String()) @@ -77,7 +80,7 @@ func TestMetadataSurvey_selectAll(t *testing.T) { assert.Equal(t, []string{"hubot"}, state.Assignees) assert.Equal(t, []string{"monalisa"}, state.Reviewers) assert.Equal(t, []string{"good first issue"}, state.Labels) - assert.Equal(t, []string{"The road to 1.0"}, state.Projects) + assert.Equal(t, []string{"The road to 1.0"}, state.ProjectTitles) assert.Equal(t, []string{}, state.Milestones) } @@ -113,7 +116,8 @@ func TestMetadataSurvey_keepExisting(t *testing.T) { state := &IssueMetadataState{ Assignees: []string{"hubot"}, } - err := MetadataSurvey(pm, ios, repo, fetcher, state) + + err := MetadataSurvey(pm, ios, repo, fetcher, state, gh.ProjectsV1Supported) assert.NoError(t, err) assert.Equal(t, "", stdout.String()) @@ -121,7 +125,64 @@ func TestMetadataSurvey_keepExisting(t *testing.T) { assert.Equal(t, []string{"hubot"}, state.Assignees) assert.Equal(t, []string{"good first issue"}, state.Labels) - assert.Equal(t, []string{"The road to 1.0"}, state.Projects) + assert.Equal(t, []string{"The road to 1.0"}, state.ProjectTitles) +} + +// TODO projectsV1Deprecation +// Remove this test and projectsV1MetadataFetcherSpy +func TestMetadataSurveyProjectV1Deprecation(t *testing.T) { + t.Run("when projectsV1 is supported, requests projectsV1", func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + repo := ghrepo.New("OWNER", "REPO") + + fetcher := &projectsV1MetadataFetcherSpy{} + pm := prompter.NewMockPrompter(t) + pm.RegisterMultiSelect("What would you like to add?", []string{}, []string{"Assignees", "Labels", "Projects", "Milestone"}, func(_ string, _, options []string) ([]int, error) { + i, err := prompter.IndexFor(options, "Projects") + require.NoError(t, err) + return []int{i}, nil + }) + pm.RegisterMultiSelect("Projects", []string{}, []string{"Huge Refactoring"}, func(_ string, _, _ []string) ([]int, error) { + return []int{0}, nil + }) + + err := MetadataSurvey(pm, ios, repo, fetcher, &IssueMetadataState{}, gh.ProjectsV1Supported) + require.ErrorContains(t, err, "expected test error") + + require.True(t, fetcher.projectsV1Requested, "expected projectsV1 to be requested") + }) + + t.Run("when projectsV1 is supported, does not request projectsV1", func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + repo := ghrepo.New("OWNER", "REPO") + + fetcher := &projectsV1MetadataFetcherSpy{} + pm := prompter.NewMockPrompter(t) + pm.RegisterMultiSelect("What would you like to add?", []string{}, []string{"Assignees", "Labels", "Projects", "Milestone"}, func(_ string, _, options []string) ([]int, error) { + i, err := prompter.IndexFor(options, "Projects") + require.NoError(t, err) + return []int{i}, nil + }) + pm.RegisterMultiSelect("Projects", []string{}, []string{"Huge Refactoring"}, func(_ string, _, _ []string) ([]int, error) { + return []int{0}, nil + }) + + err := MetadataSurvey(pm, ios, repo, fetcher, &IssueMetadataState{}, gh.ProjectsV1Unsupported) + require.ErrorContains(t, err, "expected test error") + + require.False(t, fetcher.projectsV1Requested, "expected projectsV1 not to be requested") + }) +} + +type projectsV1MetadataFetcherSpy struct { + projectsV1Requested bool +} + +func (mf *projectsV1MetadataFetcherSpy) RepoMetadataFetch(input api.RepoMetadataInput) (*api.RepoMetadataResult, error) { + if input.ProjectsV1 { + mf.projectsV1Requested = true + } + return nil, errors.New("expected test error") } func TestTitledEditSurvey_cleanupHint(t *testing.T) { diff --git a/pkg/cmd/project/shared/queries/queries.go b/pkg/cmd/project/shared/queries/queries.go index 3e63465dd..46aa58451 100644 --- a/pkg/cmd/project/shared/queries/queries.go +++ b/pkg/cmd/project/shared/queries/queries.go @@ -7,9 +7,7 @@ import ( "net/url" "regexp" "strings" - "time" - "github.com/briandowns/spinner" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/pkg/iostreams" @@ -24,8 +22,8 @@ func NewClient(httpClient *http.Client, hostname string, ios *iostreams.IOStream } return &Client{ apiClient: apiClient, - spinner: ios.IsStdoutTTY() && ios.IsStderrTTY(), - prompter: prompter.New("", ios.In, ios.Out, ios.ErrOut), + io: ios, + prompter: prompter.New("", ios), } } @@ -44,9 +42,10 @@ func NewTestClient(opts ...TestClientOpt) *Client { hostname: "github.com", Client: api.NewClientFromHTTP(http.DefaultClient), } + io, _, _, _ := iostreams.Test() c := &Client{ apiClient: apiClient, - spinner: false, + io: io, prompter: nil, } @@ -80,7 +79,7 @@ type graphqlClient interface { type Client struct { apiClient graphqlClient - spinner bool + io *iostreams.IOStreams prompter iprompter } @@ -89,19 +88,12 @@ const ( LimitMax = 100 // https://docs.github.com/en/graphql/overview/resource-limitations#node-limit ) -// doQuery wraps API calls with a visual spinner -func (c *Client) doQuery(name string, query interface{}, variables map[string]interface{}) error { - var sp *spinner.Spinner - if c.spinner { - // https://github.com/briandowns/spinner#available-character-sets - dotStyle := spinner.CharSets[11] - sp = spinner.New(dotStyle, 120*time.Millisecond, spinner.WithColor("fgCyan")) - sp.Start() - } +// doQueryWithProgressIndicator wraps API calls with a progress indicator. +// The query name is used in the progress indicator label. +func (c *Client) doQueryWithProgressIndicator(name string, query interface{}, variables map[string]interface{}) error { + c.io.StartProgressIndicatorWithLabel(fmt.Sprintf("Fetching %s", name)) + defer c.io.StopProgressIndicator() err := c.apiClient.Query(name, query, variables) - if sp != nil { - sp.Stop() - } return handleError(err) } @@ -552,7 +544,7 @@ func (c *Client) ProjectItems(o *Owner, number int32, limit int) (*Project, erro query = &viewerOwnerWithItems{} // must be a pointer to work with graphql queries queryName = "ViewerProjectWithItems" } - err := c.doQuery(queryName, query, variables) + err := c.doQueryWithProgressIndicator(queryName, query, variables) if err != nil { return project, err } @@ -706,7 +698,7 @@ func paginateAttributes[N projectAttribute](c *Client, p pager[N], variables map // set the cursor to the end of the last page variables[afterKey] = (*githubv4.String)(&cursor) - err := c.doQuery(queryName, p, variables) + err := c.doQueryWithProgressIndicator(queryName, p, variables) if err != nil { return nodes, err } @@ -863,7 +855,7 @@ func (c *Client) ProjectFields(o *Owner, number int32, limit int) (*Project, err query = &viewerOwnerWithFields{} // must be a pointer to work with graphql queries queryName = "ViewerProjectWithFields" } - err := c.doQuery(queryName, query, variables) + err := c.doQueryWithProgressIndicator(queryName, query, variables) if err != nil { return project, err } @@ -977,7 +969,7 @@ const ViewerOwner OwnerType = "VIEWER" // ViewerLoginName returns the login name of the viewer. func (c *Client) ViewerLoginName() (string, error) { var query viewerLogin - err := c.doQuery("Viewer", &query, map[string]interface{}{}) + err := c.doQueryWithProgressIndicator("Viewer", &query, map[string]interface{}{}) if err != nil { return "", err } @@ -988,7 +980,7 @@ func (c *Client) ViewerLoginName() (string, error) { func (c *Client) OwnerIDAndType(login string) (string, OwnerType, error) { if login == "@me" || login == "" { var query viewerLogin - err := c.doQuery("ViewerOwner", &query, nil) + err := c.doQueryWithProgressIndicator("ViewerOwner", &query, nil) if err != nil { return "", "", err } @@ -1009,7 +1001,7 @@ func (c *Client) OwnerIDAndType(login string) (string, OwnerType, error) { } `graphql:"organization(login: $login)"` } - err := c.doQuery("UserOrgOwner", &query, variables) + err := c.doQueryWithProgressIndicator("UserOrgOwner", &query, variables) if err != nil { // Due to the way the queries are structured, we don't know if a login belongs to a user // or to an org, even though they are unique. To deal with this, we try both - if neither @@ -1052,7 +1044,7 @@ func (c *Client) IssueOrPullRequestID(rawURL string) (string, error) { "url": githubv4.URI{URL: uri}, } var query issueOrPullRequest - err = c.doQuery("GetIssueOrPullRequest", &query, variables) + err = c.doQueryWithProgressIndicator("GetIssueOrPullRequest", &query, variables) if err != nil { return "", err } @@ -1114,7 +1106,7 @@ func (c *Client) userOrgLogins() ([]loginTypes, error) { "after": (*githubv4.String)(nil), } - err := c.doQuery("ViewerLoginAndOrgs", &v, variables) + err := c.doQueryWithProgressIndicator("ViewerLoginAndOrgs", &v, variables) if err != nil { return l, err } @@ -1152,7 +1144,7 @@ func (c *Client) paginateOrgLogins(l []loginTypes, cursor string) ([]loginTypes, "after": githubv4.String(cursor), } - err := c.doQuery("ViewerLoginAndOrgs", &v, variables) + err := c.doQueryWithProgressIndicator("ViewerLoginAndOrgs", &v, variables) if err != nil { return l, err } @@ -1247,16 +1239,16 @@ func (c *Client) NewProject(canPrompt bool, o *Owner, number int32, fields bool) if o.Type == UserOwner { var query userOwner variables["login"] = githubv4.String(o.Login) - err := c.doQuery("UserProject", &query, variables) + err := c.doQueryWithProgressIndicator("UserProject", &query, variables) return &query.Owner.Project, err } else if o.Type == OrgOwner { variables["login"] = githubv4.String(o.Login) var query orgOwner - err := c.doQuery("OrgProject", &query, variables) + err := c.doQueryWithProgressIndicator("OrgProject", &query, variables) return &query.Owner.Project, err } else if o.Type == ViewerOwner { var query viewerOwner - err := c.doQuery("ViewerProject", &query, variables) + err := c.doQueryWithProgressIndicator("ViewerProject", &query, variables) return &query.Owner.Project, err } return nil, errors.New("unknown owner type") @@ -1331,7 +1323,7 @@ func (c *Client) Projects(login string, t OwnerType, limit int, fields bool) (Pr // the cost. if t == UserOwner { var query userProjects - if err := c.doQuery("UserProjects", &query, variables); err != nil { + if err := c.doQueryWithProgressIndicator("UserProjects", &query, variables); err != nil { return projects, err } projects.Nodes = append(projects.Nodes, query.Owner.Projects.Nodes...) @@ -1340,7 +1332,7 @@ func (c *Client) Projects(login string, t OwnerType, limit int, fields bool) (Pr projects.TotalCount = query.Owner.Projects.TotalCount } else if t == OrgOwner { var query orgProjects - if err := c.doQuery("OrgProjects", &query, variables); err != nil { + if err := c.doQueryWithProgressIndicator("OrgProjects", &query, variables); err != nil { return projects, err } projects.Nodes = append(projects.Nodes, query.Owner.Projects.Nodes...) @@ -1349,7 +1341,7 @@ func (c *Client) Projects(login string, t OwnerType, limit int, fields bool) (Pr projects.TotalCount = query.Owner.Projects.TotalCount } else if t == ViewerOwner { var query viewerProjects - if err := c.doQuery("ViewerProjects", &query, variables); err != nil { + if err := c.doQueryWithProgressIndicator("ViewerProjects", &query, variables); err != nil { return projects, err } projects.Nodes = append(projects.Nodes, query.Owner.Projects.Nodes...) diff --git a/pkg/httpmock/registry.go b/pkg/httpmock/registry.go index 51aa5a898..b7c5a117d 100644 --- a/pkg/httpmock/registry.go +++ b/pkg/httpmock/registry.go @@ -7,8 +7,6 @@ import ( "strings" "sync" "testing" - - "github.com/stretchr/testify/assert" ) // Replace http.Client transport layer with registry so all requests get @@ -32,10 +30,21 @@ func (r *Registry) Register(m Matcher, resp Responder) { } func (r *Registry) Exclude(t *testing.T, m Matcher) { + registrationStack := string(debug.Stack()) + excludedStub := &Stub{ Matcher: m, Responder: func(req *http.Request) (*http.Response, error) { - assert.FailNowf(t, "Exclude error", "API called when excluded: %v", req.URL) + callStack := string(debug.Stack()) + + var errMsg strings.Builder + errMsg.WriteString("HTTP call was made when it should have been excluded:\n") + errMsg.WriteString(fmt.Sprintf("Request URL: %s\n", req.URL)) + errMsg.WriteString(fmt.Sprintf("Was excluded by: %s\n", registrationStack)) + errMsg.WriteString(fmt.Sprintf("Was called from: %s\n", callStack)) + + t.Error(errMsg.String()) + t.FailNow() return nil, nil }, exclude: true, diff --git a/pkg/iostreams/iostreams.go b/pkg/iostreams/iostreams.go index ba2cc6b50..22f966ac8 100644 --- a/pkg/iostreams/iostreams.go +++ b/pkg/iostreams/iostreams.go @@ -58,6 +58,7 @@ type IOStreams struct { progressIndicatorEnabled bool progressIndicator *spinner.Spinner progressIndicatorMu sync.Mutex + spinnerDisabled bool alternateScreenBufferEnabled bool alternateScreenBufferActive bool @@ -78,8 +79,8 @@ type IOStreams struct { pagerCommand string pagerProcess *os.Process - neverPrompt bool - spinnerDisabled bool + neverPrompt bool + accessiblePrompterEnabled bool TempFileOverride *os.File } @@ -457,6 +458,14 @@ func (s *IOStreams) AccessibleColorsEnabled() bool { return s.accessibleColorsEnabled } +func (s *IOStreams) SetAccessiblePrompterEnabled(enabled bool) { + s.accessiblePrompterEnabled = enabled +} + +func (s *IOStreams) AccessiblePrompterEnabled() bool { + return s.accessiblePrompterEnabled +} + func System() *IOStreams { terminal := ghTerm.FromEnv()