Skip to content

Early exit repo sync if merge-upstream requires workflow scope #7612

New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Merged
williammartin merged 1 commit into from
Jun 23, 2023
Merged

Early exit repo sync if merge-upstream requires workflow scope #7612

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

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions pkg/cmd/repo/sync/http.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"errors"
"fmt"
"net/http"
"regexp"

"github.com/cli/cli/v2/api"
"github.com/cli/cli/v2/internal/ghrepo"
Expand Down Expand Up @@ -33,6 +34,9 @@ type upstreamMergeErr struct{ error }

var upstreamMergeUnavailableErr = upstreamMergeErr{errors.New("upstream merge API is unavailable")}

var missingWorkflowScopeRE = regexp.MustCompile("refusing to allow.*without `workflow` scope")
var missingWorkflowScopeErr = errors.New("Upstream commits contain workflow changes, which require the `workflow` scope to merge. To request it, run: gh auth refresh -s workflow")

func triggerUpstreamMerge(client *api.Client, repo ghrepo.Interface, branch string) (string, error) {
var payload bytes.Buffer
if err := json.NewEncoder(&payload).Encode(map[string]interface{}{
Expand All @@ -52,6 +56,9 @@ func triggerUpstreamMerge(client *api.Client, repo ghrepo.Interface, branch stri
if errors.As(err, &httpErr) {
switch httpErr.StatusCode {
case http.StatusUnprocessableEntity, http.StatusConflict:
if missingWorkflowScopeRE.MatchString(httpErr.Message) {
return "", missingWorkflowScopeErr
}
return "", upstreamMergeErr{errors.New(httpErr.Message)}
case http.StatusNotFound:
return "", upstreamMergeUnavailableErr
Expand Down
18 changes: 18 additions & 0 deletions pkg/cmd/repo/sync/sync_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -457,6 +457,24 @@ func Test_SyncRun(t *testing.T) {
wantErr: true,
errMsg: "trunk branch does not exist on OWNER/REPO-FORK repository",
},
{
name: "sync remote fork with missing workflow scope on token",
opts: &SyncOptions{
DestArg: "FORKOWNER/REPO-FORK",
},
httpStubs: func(reg *httpmock.Registry) {
reg.Register(
httpmock.GraphQL(`query RepositoryInfo\b`),
httpmock.StringResponse(`{"data":{"repository":{"defaultBranchRef":{"name": "trunk"}}}}`))
reg.Register(
httpmock.REST("POST", "repos/FORKOWNER/REPO-FORK/merge-upstream"),
httpmock.StatusJSONResponse(422, struct {
Message string `json:"message"`
}{Message: "refusing to allow an OAuth App to create or update workflow `.github/workflows/unimportant.yml` without `workflow` scope"}))
},
wantErr: true,
errMsg: "Upstream commits contain workflow changes, which require the `workflow` scope to merge. To request it, run: gh auth refresh -s workflow",
},
}
for _, tt := range tests {
reg := &httpmock.Registry{}
Expand Down
21 changes: 19 additions & 2 deletions pkg/httpmock/stub.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,20 @@ func StatusStringResponse(status int, body string) Responder {
func JSONResponse(body interface{}) Responder {
return func(req *http.Request) (*http.Response, error) {
b, _ := json.Marshal(body)
return httpResponse(200, req, bytes.NewBuffer(b)), nil
header := http.Header{

@williammartin williammartin Jun 21, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Not strictly sure this was necessary but seemed sensible since it was required on the unhappy path.

"Content-Type": []string{"application/json"},
}
return httpResponseWithHeader(200, req, bytes.NewBuffer(b), header), nil
}
}

func StatusJSONResponse(status int, body interface{}) Responder {
return func(req *http.Request) (*http.Response, error) {
b, _ := json.Marshal(body)
header := http.Header{

@williammartin williammartin Jun 21, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This header was needed because the graphql error message parsing only happens if the content type is JSON

"Content-Type": []string{"application/json"},
}
return httpResponseWithHeader(status, req, bytes.NewBuffer(b), header), nil
}
}

Expand Down Expand Up @@ -215,10 +228,14 @@ func ScopesResponder(scopes string) func(*http.Request) (*http.Response, error)
}

func httpResponse(status int, req *http.Request, body io.Reader) *http.Response {
return httpResponseWithHeader(status, req, body, http.Header{})
}

func httpResponseWithHeader(status int, req *http.Request, body io.Reader, header http.Header) *http.Response {
return &http.Response{
StatusCode: status,
Request: req,
Body: io.NopCloser(body),
Header: http.Header{},
Header: header,
}
}