diff --git a/api/queries_pr.go b/api/queries_pr.go index 525418a11..1d394e864 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -85,15 +85,25 @@ type PullRequest struct { Assignees Assignees AssignedActors AssignedActors - Labels Labels - ProjectCards ProjectCards - ProjectItems ProjectItems - Milestone *Milestone - Comments Comments - ReactionGroups ReactionGroups - Reviews PullRequestReviews - LatestReviews PullRequestReviews - ReviewRequests ReviewRequests + // AssignedActorsUsed is a GIGANTIC hack to carry around whether we expected AssignedActors to be requested + // on this PR. This is required because the Feature Detection of support for AssignedActors occurs inside the + // PR Finder, but knowledge of support is required at the command level. However, we can't easily construct + // the feature detector at the command level because it needs knowledge of the BaseRepo, which is only available + // inside the PR Finder. This is bad and we should feel bad. + // + // The right solution is to extract argument parsing from the PR Finder into each command, so that we have access + // to the BaseRepo and can construct the feature detector there. This is what happens in the issue commands with + // `shared.ParseIssueFromArg`. + AssignedActorsUsed bool + Labels Labels + ProjectCards ProjectCards + ProjectItems ProjectItems + Milestone *Milestone + Comments Comments + ReactionGroups ReactionGroups + Reviews PullRequestReviews + LatestReviews PullRequestReviews + ReviewRequests ReviewRequests ClosingIssuesReferences ClosingIssuesReferences } diff --git a/go.mod b/go.mod index 2c9b32aa7..a4c973df1 100644 --- a/go.mod +++ b/go.mod @@ -14,7 +14,7 @@ require ( github.com/charmbracelet/glamour v0.9.2-0.20250319212134-549f544650e3 github.com/charmbracelet/huh v0.7.0 github.com/charmbracelet/lipgloss v1.1.1-0.20250319133953-166f707985bc - github.com/cli/go-gh/v2 v2.12.0 + github.com/cli/go-gh/v2 v2.12.1 github.com/cli/go-internal v0.0.0-20241025142207-6c48bcd5ce24 github.com/cli/oauth v1.1.1 github.com/cli/safeexec v1.0.1 diff --git a/go.sum b/go.sum index 270ede090..718e0ca67 100644 --- a/go.sum +++ b/go.sum @@ -137,8 +137,8 @@ github.com/charmbracelet/x/xpty v0.1.2/go.mod h1:XK2Z0id5rtLWcpeNiMYBccNNBrP2IJn github.com/cli/browser v1.0.0/go.mod h1:IEWkHYbLjkhtjwwWlwTHW2lGxeS5gezEQBMLTwDHf5Q= github.com/cli/browser v1.3.0 h1:LejqCrpWr+1pRqmEPDGnTZOjsMe7sehifLynZJuqJpo= github.com/cli/browser v1.3.0/go.mod h1:HH8s+fOAxjhQoBUAsKuPCbqUuxZDhQ2/aD+SzsEfBTk= -github.com/cli/go-gh/v2 v2.12.0 h1:PIurZ13fXbWDbr2//6ws4g4zDbryO+iDuTpiHgiV+6k= -github.com/cli/go-gh/v2 v2.12.0/go.mod h1:+5aXmEOJsH9fc9mBHfincDwnS02j2AIA/DsTH0Bk5uw= +github.com/cli/go-gh/v2 v2.12.1 h1:SVt1/afj5FRAythyMV3WJKaUfDNsxXTIe7arZbwTWKA= +github.com/cli/go-gh/v2 v2.12.1/go.mod h1:+5aXmEOJsH9fc9mBHfincDwnS02j2AIA/DsTH0Bk5uw= github.com/cli/go-internal v0.0.0-20241025142207-6c48bcd5ce24 h1:QDrhR4JA2n3ij9YQN0u5ZeuvRIIvsUGmf5yPlTS0w8E= github.com/cli/go-internal v0.0.0-20241025142207-6c48bcd5ce24/go.mod h1:rr9GNING0onuVw8MnracQHn7PcchnFlP882Y0II2KZk= github.com/cli/oauth v1.1.1 h1:459gD3hSjlKX9B1uXBuiAMdpXBUQ9QGf/NDcCpoQxPs= diff --git a/pkg/cmd/issue/edit/edit.go b/pkg/cmd/issue/edit/edit.go index e959cde2b..b207a96fd 100644 --- a/pkg/cmd/issue/edit/edit.go +++ b/pkg/cmd/issue/edit/edit.go @@ -171,8 +171,8 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman cmd.Flags().StringVarP(&opts.Editable.Title.Value, "title", "t", "", "Set the new title.") cmd.Flags().StringVarP(&opts.Editable.Body.Value, "body", "b", "", "Set the new body.") cmd.Flags().StringVarP(&bodyFile, "body-file", "F", "", "Read body text from `file` (use \"-\" to read from standard input)") - cmd.Flags().StringSliceVar(&opts.Editable.Assignees.Add, "add-assignee", nil, "Add assigned users by their `login`. Use \"@me\" to assign yourself.") - cmd.Flags().StringSliceVar(&opts.Editable.Assignees.Remove, "remove-assignee", nil, "Remove assigned users by their `login`. Use \"@me\" to unassign yourself.") + cmd.Flags().StringSliceVar(&opts.Editable.Assignees.Add, "add-assignee", nil, "Add assigned users by their `login`. Use \"@me\" to assign yourself, or \"@copilot\" to assign Copilot.") + cmd.Flags().StringSliceVar(&opts.Editable.Assignees.Remove, "remove-assignee", nil, "Remove assigned users by their `login`. Use \"@me\" to unassign yourself, or \"@copilot\" to unassign Copilot.") cmd.Flags().StringSliceVar(&opts.Editable.Labels.Add, "add-label", nil, "Add labels by `name`") cmd.Flags().StringSliceVar(&opts.Editable.Labels.Remove, "remove-label", nil, "Remove labels by `name`") cmd.Flags().StringSliceVar(&opts.Editable.Projects.Add, "add-project", nil, "Add the issue to projects by `title`") diff --git a/pkg/cmd/pr/edit/edit.go b/pkg/cmd/pr/edit/edit.go index c30c4f9bb..5e1f19b18 100644 --- a/pkg/cmd/pr/edit/edit.go +++ b/pkg/cmd/pr/edit/edit.go @@ -3,7 +3,6 @@ package edit import ( "fmt" "net/http" - "time" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" @@ -171,8 +170,8 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman cmd.Flags().StringVarP(&opts.Editable.Base.Value, "base", "B", "", "Change the base `branch` for this pull request") cmd.Flags().StringSliceVar(&opts.Editable.Reviewers.Add, "add-reviewer", nil, "Add reviewers by their `login`.") cmd.Flags().StringSliceVar(&opts.Editable.Reviewers.Remove, "remove-reviewer", nil, "Remove reviewers by their `login`.") - cmd.Flags().StringSliceVar(&opts.Editable.Assignees.Add, "add-assignee", nil, "Add assigned users by their `login`. Use \"@me\" to assign yourself.") - cmd.Flags().StringSliceVar(&opts.Editable.Assignees.Remove, "remove-assignee", nil, "Remove assigned users by their `login`. Use \"@me\" to unassign yourself.") + cmd.Flags().StringSliceVar(&opts.Editable.Assignees.Add, "add-assignee", nil, "Add assigned users by their `login`. Use \"@me\" to assign yourself, or \"@copilot\" to assign Copilot.") + cmd.Flags().StringSliceVar(&opts.Editable.Assignees.Remove, "remove-assignee", nil, "Remove assigned users by their `login`. Use \"@me\" to unassign yourself, or \"@copilot\" to unassign Copilot.") cmd.Flags().StringSliceVar(&opts.Editable.Labels.Add, "add-label", nil, "Add labels by `name`") cmd.Flags().StringSliceVar(&opts.Editable.Labels.Remove, "remove-label", nil, "Remove labels by `name`") cmd.Flags().StringSliceVar(&opts.Editable.Projects.Add, "add-project", nil, "Add the pull request to projects by `title`") @@ -206,7 +205,7 @@ func NewCmdEdit(f *cmdutil.Factory, runF func(*EditOptions) error) *cobra.Comman func editRun(opts *EditOptions) error { findOptions := shared.FindOptions{ Selector: opts.SelectorArg, - Fields: []string{"id", "url", "title", "body", "baseRefName", "reviewRequests", "labels", "projectCards", "projectItems", "milestone"}, + Fields: []string{"id", "url", "title", "body", "baseRefName", "reviewRequests", "labels", "projectCards", "projectItems", "milestone", "assignees"}, Detector: opts.Detector, } @@ -215,27 +214,6 @@ func editRun(opts *EditOptions) error { return err } - if opts.Detector == nil { - baseRepo, err := opts.BaseRepo() - if err != nil { - return err - } - - cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24) - opts.Detector = fd.NewDetector(cachedClient, baseRepo.RepoHost()) - } - - issueFeatures, err := opts.Detector.IssueFeatures() - if err != nil { - return err - } - - if issueFeatures.ActorIsAssignable { - findOptions.Fields = append(findOptions.Fields, "assignedActors") - } else { - findOptions.Fields = append(findOptions.Fields, "assignees") - } - pr, repo, err := opts.Finder.Find(findOptions) if err != nil { return err @@ -247,7 +225,7 @@ func editRun(opts *EditOptions) error { editable.Body.Default = pr.Body editable.Base.Default = pr.BaseRefName editable.Reviewers.Default = pr.ReviewRequests.Logins() - if issueFeatures.ActorIsAssignable { + if pr.AssignedActorsUsed { editable.Assignees.ActorAssignees = true editable.Assignees.Default = pr.AssignedActors.DisplayNames() } else { diff --git a/pkg/cmd/pr/edit/edit_test.go b/pkg/cmd/pr/edit/edit_test.go index 3a1e8a548..0c40e3839 100644 --- a/pkg/cmd/pr/edit/edit_test.go +++ b/pkg/cmd/pr/edit/edit_test.go @@ -341,9 +341,11 @@ func Test_editRun(t *testing.T) { { name: "non-interactive", input: &EditOptions{ + Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", Finder: shared.NewMockFinder("123", &api.PullRequest{ - URL: "https://github.com/OWNER/REPO/pull/123", + URL: "https://github.com/OWNER/REPO/pull/123", + AssignedActorsUsed: true, }, ghrepo.New("OWNER", "REPO")), Interactive: false, Editable: shared.Editable{ @@ -403,9 +405,11 @@ func Test_editRun(t *testing.T) { { name: "non-interactive skip reviewers", input: &EditOptions{ + Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", Finder: shared.NewMockFinder("123", &api.PullRequest{ - URL: "https://github.com/OWNER/REPO/pull/123", + URL: "https://github.com/OWNER/REPO/pull/123", + AssignedActorsUsed: true, }, ghrepo.New("OWNER", "REPO")), Interactive: false, Editable: shared.Editable{ @@ -459,9 +463,11 @@ func Test_editRun(t *testing.T) { { name: "non-interactive remove all reviewers", input: &EditOptions{ + Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", Finder: shared.NewMockFinder("123", &api.PullRequest{ - URL: "https://github.com/OWNER/REPO/pull/123", + URL: "https://github.com/OWNER/REPO/pull/123", + AssignedActorsUsed: true, }, ghrepo.New("OWNER", "REPO")), Interactive: false, Editable: shared.Editable{ @@ -520,9 +526,11 @@ func Test_editRun(t *testing.T) { { name: "interactive", input: &EditOptions{ + Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", Finder: shared.NewMockFinder("123", &api.PullRequest{ - URL: "https://github.com/OWNER/REPO/pull/123", + URL: "https://github.com/OWNER/REPO/pull/123", + AssignedActorsUsed: true, }, ghrepo.New("OWNER", "REPO")), Interactive: true, Surveyor: testSurveyor{}, @@ -542,9 +550,11 @@ func Test_editRun(t *testing.T) { { name: "interactive skip reviewers", input: &EditOptions{ + Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", Finder: shared.NewMockFinder("123", &api.PullRequest{ - URL: "https://github.com/OWNER/REPO/pull/123", + URL: "https://github.com/OWNER/REPO/pull/123", + AssignedActorsUsed: true, }, ghrepo.New("OWNER", "REPO")), Interactive: true, Surveyor: testSurveyor{skipReviewers: true}, @@ -563,9 +573,11 @@ func Test_editRun(t *testing.T) { { name: "interactive remove all reviewers", input: &EditOptions{ + Detector: &fd.EnabledDetectorMock{}, SelectorArg: "123", Finder: shared.NewMockFinder("123", &api.PullRequest{ - URL: "https://github.com/OWNER/REPO/pull/123", + URL: "https://github.com/OWNER/REPO/pull/123", + AssignedActorsUsed: true, }, ghrepo.New("OWNER", "REPO")), Interactive: true, Surveyor: testSurveyor{removeAllReviewers: true}, diff --git a/pkg/cmd/pr/shared/finder.go b/pkg/cmd/pr/shared/finder.go index b8d23c978..a19c69669 100644 --- a/pkg/cmd/pr/shared/finder.go +++ b/pkg/cmd/pr/shared/finder.go @@ -242,6 +242,33 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err } } + // Ok this is super, super horrible so bear with me. + // The `assignees` field on a Pull Request exposes users that are assigned. It is also possible for bots to be + // assigned, but they only appear under the `assignedActors` field. Ideally, the caller of `Find` would determine + // the correct field to use based on the `fd.Detector` that is passed in, but they can't construct a detector + // because the BaseRepo is only determined within this function. The more correct solution is to do what I did with + // the issue commands and decouple argument parsing from API lookup. See PR #10811 for example. + var actorAssigneesUsed bool + if fields.Contains("assignees") { + if opts.Detector == nil { + cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24) + opts.Detector = fd.NewDetector(cachedClient, f.baseRefRepo.RepoHost()) + } + + issueFeatures, err := opts.Detector.IssueFeatures() + if err != nil { + return nil, nil, fmt.Errorf("error detecting issue features: %v", err) + } + + // If actors are assignable on this host then we additionally request the `assignedActors` field. + // Note that we don't remove the `assignees` field because some commands (`pr view`) do not display actor + // assignees yet, so we have to have both sets of data. + if issueFeatures.ActorIsAssignable { + fields.Add("assignedActors") + actorAssigneesUsed = true + } + } + var pr *api.PullRequest if f.prNumber > 0 { // If we have a PR number, let's look it up @@ -297,6 +324,10 @@ func (f *finder) Find(opts FindOptions) (*api.PullRequest, ghrepo.Interface, err }) } + if actorAssigneesUsed { + pr.AssignedActorsUsed = true + } + return pr, f.baseRefRepo, g.Wait() } diff --git a/pkg/cmd/pr/shared/finder_test.go b/pkg/cmd/pr/shared/finder_test.go index abc754d1a..28c2337d3 100644 --- a/pkg/cmd/pr/shared/finder_test.go +++ b/pkg/cmd/pr/shared/finder_test.go @@ -9,6 +9,7 @@ import ( ghContext "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/git" + fd "github.com/cli/cli/v2/internal/featuredetection" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/httpmock" "github.com/stretchr/testify/require" @@ -705,6 +706,81 @@ func TestFind(t *testing.T) { } } +func TestFindAssignableActors(t *testing.T) { + t.Run("given actors are not assignable, do nothing special", func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + // Ensure we never request assignedActors + reg.Exclude(t, httpmock.GraphQL(`assignedActors`)) + reg.Register( + httpmock.GraphQL(`query PullRequestByNumber\b`), + httpmock.StringResponse(`{"data":{"repository":{ + "pullRequest":{"number":13} + }}}`)) + + f := finder{ + httpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + } + + pr, _, err := f.Find(FindOptions{ + Detector: &fd.DisabledDetectorMock{}, + Fields: []string{"assignees"}, + Selector: "https://github.com/cli/cli/pull/123", + }) + require.NoError(t, err) + + require.False(t, pr.AssignedActorsUsed, "expected PR not to have assigned actors used") + }) + + t.Run("given actors are assignable, request assignedActors and indicate that on the returned PR", func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + // Ensure that we only respond if assignedActors is requested + reg.Register( + httpmock.GraphQL(`assignedActors`), + httpmock.StringResponse(`{"data":{"repository":{ + "pullRequest":{ + "number":13, + "assignedActors": { + "nodes": [ + { + "id": "HUBOTID", + "login": "hubot", + "__typename": "Bot" + }, + { + "id": "MONAID", + "login": "MonaLisa", + "name": "Mona Display Name", + "__typename": "User" + } + ], + "totalCount": 2 + }} + }}}`)) + + f := finder{ + httpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + } + + pr, _, err := f.Find(FindOptions{ + Detector: &fd.EnabledDetectorMock{}, + Fields: []string{"assignees"}, + Selector: "https://github.com/cli/cli/pull/123", + }) + require.NoError(t, err) + + require.Equal(t, []string{"hubot", "MonaLisa"}, pr.AssignedActors.Logins()) + require.True(t, pr.AssignedActorsUsed, "expected PR to have assigned actors used") + }) +} + func stubBranchConfig(branchConfig git.BranchConfig, err error) func(context.Context, string) (git.BranchConfig, error) { return func(_ context.Context, branch string) (git.BranchConfig, error) { return branchConfig, err diff --git a/pkg/cmd/release/shared/fetch.go b/pkg/cmd/release/shared/fetch.go index 8db7e502a..4c0a014b9 100644 --- a/pkg/cmd/release/shared/fetch.go +++ b/pkg/cmd/release/shared/fetch.go @@ -71,6 +71,7 @@ type ReleaseAsset struct { Name string Label string Size int64 + Digest *string State string APIURL string `json:"url"` @@ -107,6 +108,7 @@ func (rel *Release) ExportData(fields []string) map[string]interface{} { "name": a.Name, "label": a.Label, "size": a.Size, + "digest": a.Digest, "state": a.State, "createdAt": a.CreatedAt, "updatedAt": a.UpdatedAt, diff --git a/pkg/cmd/release/view/view.go b/pkg/cmd/release/view/view.go index c9030f299..59a19b4e5 100644 --- a/pkg/cmd/release/view/view.go +++ b/pkg/cmd/release/view/view.go @@ -154,10 +154,14 @@ func renderReleaseTTY(io *iostreams.IOStreams, release *shared.Release) error { if len(release.Assets) > 0 { fmt.Fprintln(w, cs.Bold("Assets")) - //nolint:staticcheck // SA1019: Showing NAME|SIZE headers adds nothing to table. - table := tableprinter.New(io, tableprinter.NoHeader) + table := tableprinter.New(io, tableprinter.WithHeader("Name", "Digest", "Size")) for _, a := range release.Assets { table.AddField(a.Name) + if a.Digest == nil { + table.AddField("") + } else { + table.AddField(*a.Digest) + } table.AddField(humanFileSize(a.Size)) table.EndRow() } diff --git a/pkg/cmd/release/view/view_test.go b/pkg/cmd/release/view/view_test.go index 8ca4f14c8..be345b186 100644 --- a/pkg/cmd/release/view/view_test.go +++ b/pkg/cmd/release/view/view_test.go @@ -150,8 +150,9 @@ func Test_viewRun(t *testing.T) { Assets - windows.zip 12 B - linux.tgz 34 B + NAME DIGEST SIZE + windows.zip sha256:deadc0de 12 B + linux.tgz 34 B View on GitHub: https://github.com/OWNER/REPO/releases/tags/v1.2.3 `), @@ -174,8 +175,9 @@ func Test_viewRun(t *testing.T) { Assets - windows.zip 12 B - linux.tgz 34 B + NAME DIGEST SIZE + windows.zip sha256:deadc0de 12 B + linux.tgz 34 B View on GitHub: https://github.com/OWNER/REPO/releases/tags/v1.2.3 `), @@ -248,8 +250,8 @@ func Test_viewRun(t *testing.T) { "published_at": "%[1]s", "html_url": "https://github.com/OWNER/REPO/releases/tags/v1.2.3", "assets": [ - { "name": "windows.zip", "size": 12 }, - { "name": "linux.tgz", "size": 34 } + { "name": "windows.zip", "size": 12, "digest": "sha256:deadc0de" }, + { "name": "linux.tgz", "size": 34, "digest": null } ] }`, tt.releasedAt.Format(time.RFC3339), tt.releaseBody))