From 1e295607d7e703f8db5d34ba8d352dfe073394ed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 14 Sep 2022 16:19:33 +0200 Subject: [PATCH 1/2] Fix deleting remote branches with `#` in their name --- api/queries_pr.go | 3 ++- api/queries_pr_test.go | 40 ++++++++++++++++++++++++---------------- pkg/httpmock/stub.go | 2 +- 3 files changed, 27 insertions(+), 18 deletions(-) diff --git a/api/queries_pr.go b/api/queries_pr.go index 18217a583..70f5494a5 100644 --- a/api/queries_pr.go +++ b/api/queries_pr.go @@ -3,6 +3,7 @@ package api import ( "fmt" "net/http" + "net/url" "time" "github.com/cli/cli/v2/internal/ghrepo" @@ -447,6 +448,6 @@ func PullRequestReady(client *Client, repo ghrepo.Interface, pr *PullRequest) er } func BranchDeleteRemote(client *Client, repo ghrepo.Interface, branch string) error { - path := fmt.Sprintf("repos/%s/%s/git/refs/heads/%s", repo.RepoOwner(), repo.RepoName(), branch) + path := fmt.Sprintf("repos/%s/%s/git/refs/heads/%s", repo.RepoOwner(), repo.RepoName(), url.PathEscape(branch)) return client.REST(repo.RepoHost(), "DELETE", path, nil, nil) } diff --git a/api/queries_pr_test.go b/api/queries_pr_test.go index 9f0ed2ff0..0e2646048 100644 --- a/api/queries_pr_test.go +++ b/api/queries_pr_test.go @@ -11,36 +11,44 @@ import ( func TestBranchDeleteRemote(t *testing.T) { var tests = []struct { - name string - responseStatus int - responseBody string - expectError bool + name string + branch string + httpStubs func(*httpmock.Registry) + expectError bool }{ { - name: "success", - responseStatus: 204, - responseBody: "", - expectError: false, + name: "success", + branch: "owner/branch#123", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/owner%2Fbranch%23123"), + httpmock.StatusStringResponse(204, "")) + }, + expectError: false, }, { - name: "error", - responseStatus: 500, - responseBody: `{"message": "oh no"}`, - expectError: true, + name: "error", + branch: "my-branch", + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/my-branch"), + httpmock.StatusStringResponse(500, `{"message": "oh no"}`)) + }, + expectError: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { http := &httpmock.Registry{} - http.Register( - httpmock.REST("DELETE", "repos/OWNER/REPO/git/refs/heads/branch"), - httpmock.StatusStringResponse(tt.responseStatus, tt.responseBody)) + if tt.httpStubs != nil { + tt.httpStubs(http) + } client := newTestClient(http) repo, _ := ghrepo.FromFullName("OWNER/REPO") - err := BranchDeleteRemote(client, repo, "branch") + err := BranchDeleteRemote(client, repo, tt.branch) if (err != nil) != tt.expectError { t.Fatalf("unexpected result: %v", err) } diff --git a/pkg/httpmock/stub.go b/pkg/httpmock/stub.go index 2738bd568..cae241516 100644 --- a/pkg/httpmock/stub.go +++ b/pkg/httpmock/stub.go @@ -29,7 +29,7 @@ func REST(method, p string) Matcher { if !strings.EqualFold(req.Method, method) { return false } - return req.URL.Path == "/"+p + return req.URL.EscapedPath() == "/"+p } } From 984cc9f441f0236b28fe6af4660b5a3901cacef9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Wed, 14 Sep 2022 17:05:43 +0200 Subject: [PATCH 2/2] Fix tests --- pkg/cmd/workflow/disable/disable_test.go | 8 ++++---- pkg/cmd/workflow/enable/enable_test.go | 12 ++++++------ pkg/cmd/workflow/shared/shared.go | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/pkg/cmd/workflow/disable/disable_test.go b/pkg/cmd/workflow/disable/disable_test.go index 82d0bba2f..ea6016092 100644 --- a/pkg/cmd/workflow/disable/disable_test.go +++ b/pkg/cmd/workflow/disable/disable_test.go @@ -133,7 +133,7 @@ func TestDisableRun(t *testing.T) { tty: true, httpStubs: func(reg *httpmock.Registry) { reg.Register( - httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/a workflow"), + httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/a%20workflow"), httpmock.StatusStringResponse(404, "not found")) reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows"), @@ -158,7 +158,7 @@ func TestDisableRun(t *testing.T) { tty: true, httpStubs: func(reg *httpmock.Registry) { reg.Register( - httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/another workflow"), + httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/another%20workflow"), httpmock.StatusStringResponse(404, "not found")) reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows"), @@ -217,7 +217,7 @@ func TestDisableRun(t *testing.T) { }, httpStubs: func(reg *httpmock.Registry) { reg.Register( - httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/a workflow"), + httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/a%20workflow"), httpmock.StatusStringResponse(404, "not found")) reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows"), @@ -242,7 +242,7 @@ func TestDisableRun(t *testing.T) { }, httpStubs: func(reg *httpmock.Registry) { reg.Register( - httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/another workflow"), + httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/another%20workflow"), httpmock.StatusStringResponse(404, "not found")) reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows"), diff --git a/pkg/cmd/workflow/enable/enable_test.go b/pkg/cmd/workflow/enable/enable_test.go index 2c3db074d..7e0f29031 100644 --- a/pkg/cmd/workflow/enable/enable_test.go +++ b/pkg/cmd/workflow/enable/enable_test.go @@ -133,7 +133,7 @@ func TestEnableRun(t *testing.T) { tty: true, httpStubs: func(reg *httpmock.Registry) { reg.Register( - httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/terrible workflow"), + httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/terrible%20workflow"), httpmock.StatusStringResponse(404, "not found")) reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows"), @@ -159,7 +159,7 @@ func TestEnableRun(t *testing.T) { tty: true, httpStubs: func(reg *httpmock.Registry) { reg.Register( - httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/a disabled workflow"), + httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/a%20disabled%20workflow"), httpmock.StatusStringResponse(404, "not found")) reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows"), @@ -188,7 +188,7 @@ func TestEnableRun(t *testing.T) { tty: true, httpStubs: func(reg *httpmock.Registry) { reg.Register( - httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/a disabled inactivity workflow"), + httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/a%20disabled%20inactivity%20workflow"), httpmock.StatusStringResponse(404, "not found")) reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows"), @@ -243,7 +243,7 @@ func TestEnableRun(t *testing.T) { }, httpStubs: func(reg *httpmock.Registry) { reg.Register( - httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/terrible workflow"), + httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/terrible%20workflow"), httpmock.StatusStringResponse(404, "not found")) reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows"), @@ -268,7 +268,7 @@ func TestEnableRun(t *testing.T) { }, httpStubs: func(reg *httpmock.Registry) { reg.Register( - httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/a disabled inactivity workflow"), + httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/a%20disabled%20inactivity%20workflow"), httpmock.StatusStringResponse(404, "not found")) reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows"), @@ -292,7 +292,7 @@ func TestEnableRun(t *testing.T) { }, httpStubs: func(reg *httpmock.Registry) { reg.Register( - httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/a disabled workflow"), + httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows/a%20disabled%20workflow"), httpmock.StatusStringResponse(404, "not found")) reg.Register( httpmock.REST("GET", "repos/OWNER/REPO/actions/workflows"), diff --git a/pkg/cmd/workflow/shared/shared.go b/pkg/cmd/workflow/shared/shared.go index efef97e1c..4a276f487 100644 --- a/pkg/cmd/workflow/shared/shared.go +++ b/pkg/cmd/workflow/shared/shared.go @@ -139,7 +139,7 @@ func getWorkflowByID(client *api.Client, repo ghrepo.Interface, ID string) (*Wor var workflow Workflow err := client.REST(repo.RepoHost(), "GET", - fmt.Sprintf("repos/%s/actions/workflows/%s", ghrepo.FullName(repo), ID), + fmt.Sprintf("repos/%s/actions/workflows/%s", ghrepo.FullName(repo), url.PathEscape(ID)), nil, &workflow) if err != nil {