From ac348b0dec50b5dff69f9743ea43e45c6a5e08d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 19 Apr 2021 12:08:57 +0200 Subject: [PATCH] Fix requesting REST sub-resources on GHE GitHub REST resources typically return full URLs to fetch related resources at. We used to parse those URLs to find just the path portion and pass that in to the `REST()` function, which only accepted paths. By doing so, we are essential de-constructing a URL just to re-assemble it again. While re-assembling it for Enterprise, though, we would accidentally inject an extra `api/v3/` prefix where one was not needed. The solution is just to use raw URLs as reported by the REST API with no modifications. This extends the `REST()` function to accept full URLs in addition to just paths to resources. --- api/client.go | 10 ++++++++-- api/client_test.go | 20 ++++++++++++++++++++ pkg/cmd/run/shared/shared.go | 8 +------- pkg/cmd/run/shared/test.go | 8 ++++---- pkg/cmd/run/view/view_test.go | 30 +++++++++++++++--------------- pkg/cmd/secret/list/list.go | 9 +-------- 6 files changed, 49 insertions(+), 36 deletions(-) diff --git a/api/client.go b/api/client.go index 5d8265ab5..21cb3febc 100644 --- a/api/client.go +++ b/api/client.go @@ -194,8 +194,7 @@ func graphQLClient(h *http.Client, hostname string) *graphql.Client { // REST performs a REST request and parses the response. func (c Client) REST(hostname string, method string, p string, body io.Reader, data interface{}) error { - url := ghinstance.RESTPrefix(hostname) + p - req, err := http.NewRequest(method, url, body) + req, err := http.NewRequest(method, restURL(hostname, p), body) if err != nil { return err } @@ -230,6 +229,13 @@ func (c Client) REST(hostname string, method string, p string, body io.Reader, d return nil } +func restURL(hostname string, pathOrURL string) string { + if strings.HasPrefix(pathOrURL, "https://") || strings.HasPrefix(pathOrURL, "http://") { + return pathOrURL + } + return ghinstance.RESTPrefix(hostname) + pathOrURL +} + func handleResponse(resp *http.Response, data interface{}) error { success := resp.StatusCode >= 200 && resp.StatusCode < 300 diff --git a/api/client_test.go b/api/client_test.go index 29da3363a..df0fdf8e2 100644 --- a/api/client_test.go +++ b/api/client_test.go @@ -80,6 +80,26 @@ func TestRESTGetDelete(t *testing.T) { assert.NoError(t, err) } +func TestRESTWithFullURL(t *testing.T) { + http := &httpmock.Registry{} + client := NewClient(ReplaceTripper(http)) + + http.Register( + httpmock.REST("GET", "api/v3/user/repos"), + httpmock.StatusStringResponse(200, "{}")) + http.Register( + httpmock.REST("GET", "user/repos"), + httpmock.StatusStringResponse(200, "{}")) + + err := client.REST("example.com", "GET", "user/repos", nil, nil) + assert.NoError(t, err) + err = client.REST("example.com", "GET", "https://another.net/user/repos", nil, nil) + assert.NoError(t, err) + + assert.Equal(t, "example.com", http.Requests[0].URL.Hostname()) + assert.Equal(t, "another.net", http.Requests[1].URL.Hostname()) +} + func TestRESTError(t *testing.T) { fakehttp := &httpmock.Registry{} client := NewClient(ReplaceTripper(fakehttp)) diff --git a/pkg/cmd/run/shared/shared.go b/pkg/cmd/run/shared/shared.go index 9ca0c48f8..ea03aef6f 100644 --- a/pkg/cmd/run/shared/shared.go +++ b/pkg/cmd/run/shared/shared.go @@ -243,13 +243,7 @@ type JobsPayload struct { func GetJobs(client *api.Client, repo ghrepo.Interface, run Run) ([]Job, error) { var result JobsPayload - parsed, err := url.Parse(run.JobsURL) - if err != nil { - return nil, err - } - - err = client.REST(repo.RepoHost(), "GET", parsed.Path[1:], nil, &result) - if err != nil { + if err := client.REST(repo.RepoHost(), "GET", run.JobsURL, nil, &result); err != nil { return nil, err } return result.Jobs, nil diff --git a/pkg/cmd/run/shared/test.go b/pkg/cmd/run/shared/test.go index e0373d828..b4eff848d 100644 --- a/pkg/cmd/run/shared/test.go +++ b/pkg/cmd/run/shared/test.go @@ -26,12 +26,12 @@ func TestRun(name string, id int, s Status, c Conclusion) Run { Conclusion: c, Event: "push", HeadBranch: "trunk", - JobsURL: fmt.Sprintf("/runs/%d/jobs", id), + JobsURL: fmt.Sprintf("https://api.github.com/runs/%d/jobs", id), HeadCommit: Commit{ Message: "cool commit", }, HeadSha: "1234567890", - URL: fmt.Sprintf("runs/%d", id), + URL: fmt.Sprintf("https://github.com/runs/%d", id), HeadRepository: Repo{ Owner: struct{ Login string }{Login: "OWNER"}, Name: "REPO", @@ -68,7 +68,7 @@ var SuccessfulJob Job = Job{ Name: "cool job", StartedAt: created(), CompletedAt: updated(), - URL: "jobs/10", + URL: "https://github.com/jobs/10", RunID: 3, Steps: []Step{ { @@ -93,7 +93,7 @@ var FailedJob Job = Job{ Name: "sad job", StartedAt: created(), CompletedAt: updated(), - URL: "jobs/20", + URL: "https://github.com/jobs/20", RunID: 1234, Steps: []Step{ { diff --git a/pkg/cmd/run/view/view_test.go b/pkg/cmd/run/view/view_test.go index 8ddde1cd0..807481ca3 100644 --- a/pkg/cmd/run/view/view_test.go +++ b/pkg/cmd/run/view/view_test.go @@ -207,7 +207,7 @@ func TestViewRun(t *testing.T) { httpmock.REST("GET", "repos/OWNER/REPO/check-runs/10/annotations"), httpmock.JSONResponse([]shared.Annotation{})) }, - wantOut: "\n✓ trunk successful #2898 · 3\nTriggered via push about 59 minutes ago\n\nJOBS\n✓ cool job in 4m34s (ID 10)\n\nFor more information about a job, try: gh run view --job=\nView this run on GitHub: runs/3\n", + wantOut: "\n✓ trunk successful #2898 · 3\nTriggered via push about 59 minutes ago\n\nJOBS\n✓ cool job in 4m34s (ID 10)\n\nFor more information about a job, try: gh run view --job=\nView this run on GitHub: https://github.com/runs/3\n", }, { name: "exit status, failed run", @@ -236,7 +236,7 @@ func TestViewRun(t *testing.T) { httpmock.REST("GET", "repos/OWNER/REPO/check-runs/20/annotations"), httpmock.JSONResponse(shared.FailedJobAnnotations)) }, - wantOut: "\nX trunk failed · 1234\nTriggered via push about 59 minutes ago\n\nJOBS\nX sad job in 4m34s (ID 20)\n ✓ barf the quux\n X quux the barf\n\nANNOTATIONS\nX the job is sad\nsad job: blaze.py#420\n\n\nTo see what failed, try: gh run view 1234 --log-failed\nView this run on GitHub: runs/1234\n", + wantOut: "\nX trunk failed · 1234\nTriggered via push about 59 minutes ago\n\nJOBS\nX sad job in 4m34s (ID 20)\n ✓ barf the quux\n X quux the barf\n\nANNOTATIONS\nX the job is sad\nsad job: blaze.py#420\n\n\nTo see what failed, try: gh run view 1234 --log-failed\nView this run on GitHub: https://github.com/runs/1234\n", wantErr: true, }, { @@ -278,7 +278,7 @@ func TestViewRun(t *testing.T) { artifact-3 For more information about a job, try: gh run view --job= - View this run on GitHub: runs/3 + View this run on GitHub: https://github.com/runs/3 `), }, { @@ -308,7 +308,7 @@ func TestViewRun(t *testing.T) { httpmock.REST("GET", "repos/OWNER/REPO/check-runs/10/annotations"), httpmock.JSONResponse([]shared.Annotation{})) }, - wantOut: "\n✓ trunk successful · 3\nTriggered via push about 59 minutes ago\n\nJOBS\n✓ cool job in 4m34s (ID 10)\n\nFor more information about a job, try: gh run view --job=\nView this run on GitHub: runs/3\n", + wantOut: "\n✓ trunk successful · 3\nTriggered via push about 59 minutes ago\n\nJOBS\n✓ cool job in 4m34s (ID 10)\n\nFor more information about a job, try: gh run view --job=\nView this run on GitHub: https://github.com/runs/3\n", }, { name: "verbose", @@ -343,7 +343,7 @@ func TestViewRun(t *testing.T) { httpmock.REST("GET", "repos/OWNER/REPO/check-runs/20/annotations"), httpmock.JSONResponse(shared.FailedJobAnnotations)) }, - wantOut: "\nX trunk failed · 1234\nTriggered via push about 59 minutes ago\n\nJOBS\n✓ cool job in 4m34s (ID 10)\n ✓ fob the barz\n ✓ barz the fob\nX sad job in 4m34s (ID 20)\n ✓ barf the quux\n X quux the barf\n\nANNOTATIONS\nX the job is sad\nsad job: blaze.py#420\n\n\nTo see what failed, try: gh run view 1234 --log-failed\nView this run on GitHub: runs/1234\n", + wantOut: "\nX trunk failed · 1234\nTriggered via push about 59 minutes ago\n\nJOBS\n✓ cool job in 4m34s (ID 10)\n ✓ fob the barz\n ✓ barz the fob\nX sad job in 4m34s (ID 20)\n ✓ barf the quux\n X quux the barf\n\nANNOTATIONS\nX the job is sad\nsad job: blaze.py#420\n\n\nTo see what failed, try: gh run view 1234 --log-failed\nView this run on GitHub: https://github.com/runs/1234\n", }, { name: "prompts for choice, one job", @@ -380,7 +380,7 @@ func TestViewRun(t *testing.T) { opts: &ViewOptions{ Prompt: true, }, - wantOut: "\n✓ trunk successful · 3\nTriggered via push about 59 minutes ago\n\nJOBS\n✓ cool job in 4m34s (ID 10)\n\nFor more information about a job, try: gh run view --job=\nView this run on GitHub: runs/3\n", + wantOut: "\n✓ trunk successful · 3\nTriggered via push about 59 minutes ago\n\nJOBS\n✓ cool job in 4m34s (ID 10)\n\nFor more information about a job, try: gh run view --job=\nView this run on GitHub: https://github.com/runs/3\n", }, { name: "interactive with log", @@ -664,7 +664,7 @@ func TestViewRun(t *testing.T) { httpmock.REST("GET", "repos/OWNER/REPO/check-runs/10/annotations"), httpmock.JSONResponse([]shared.Annotation{})) }, - wantOut: "\n✓ trunk successful · 3\nTriggered via push about 59 minutes ago\n\n✓ cool job in 4m34s (ID 10)\n ✓ fob the barz\n ✓ barz the fob\n\nTo see the full job log, try: gh run view --log --job=10\nView this run on GitHub: runs/3\n", + wantOut: "\n✓ trunk successful · 3\nTriggered via push about 59 minutes ago\n\n✓ cool job in 4m34s (ID 10)\n ✓ fob the barz\n ✓ barz the fob\n\nTo see the full job log, try: gh run view --log --job=10\nView this run on GitHub: https://github.com/runs/3\n", }, { name: "interactive, multiple jobs, choose all jobs", @@ -703,7 +703,7 @@ func TestViewRun(t *testing.T) { as.StubOne(2) as.StubOne(0) }, - wantOut: "\n✓ trunk successful · 3\nTriggered via push about 59 minutes ago\n\nJOBS\n✓ cool job in 4m34s (ID 10)\nX sad job in 4m34s (ID 20)\n ✓ barf the quux\n X quux the barf\n\nANNOTATIONS\nX the job is sad\nsad job: blaze.py#420\n\n\nFor more information about a job, try: gh run view --job=\nView this run on GitHub: runs/3\n", + wantOut: "\n✓ trunk successful · 3\nTriggered via push about 59 minutes ago\n\nJOBS\n✓ cool job in 4m34s (ID 10)\nX sad job in 4m34s (ID 20)\n ✓ barf the quux\n X quux the barf\n\nANNOTATIONS\nX the job is sad\nsad job: blaze.py#420\n\n\nFor more information about a job, try: gh run view --job=\nView this run on GitHub: https://github.com/runs/3\n", }, { name: "interactive, multiple jobs, choose specific jobs", @@ -736,7 +736,7 @@ func TestViewRun(t *testing.T) { as.StubOne(2) as.StubOne(1) }, - wantOut: "\n✓ trunk successful · 3\nTriggered via push about 59 minutes ago\n\n✓ cool job in 4m34s (ID 10)\n ✓ fob the barz\n ✓ barz the fob\n\nTo see the full job log, try: gh run view --log --job=10\nView this run on GitHub: runs/3\n", + wantOut: "\n✓ trunk successful · 3\nTriggered via push about 59 minutes ago\n\n✓ cool job in 4m34s (ID 10)\n ✓ fob the barz\n ✓ barz the fob\n\nTo see the full job log, try: gh run view --log --job=10\nView this run on GitHub: https://github.com/runs/3\n", }, { name: "web run", @@ -750,8 +750,8 @@ func TestViewRun(t *testing.T) { httpmock.REST("GET", "repos/OWNER/REPO/actions/runs/3"), httpmock.JSONResponse(shared.SuccessfulRun)) }, - browsedURL: "runs/3", - wantOut: "Opening runs/3 in your browser.\n", + browsedURL: "https://github.com/runs/3", + wantOut: "Opening github.com/runs/3 in your browser.\n", }, { name: "web job", @@ -768,8 +768,8 @@ func TestViewRun(t *testing.T) { httpmock.REST("GET", "repos/OWNER/REPO/actions/runs/3"), httpmock.JSONResponse(shared.SuccessfulRun)) }, - browsedURL: "jobs/10?check_suite_focus=true", - wantOut: "Opening jobs/10 in your browser.\n", + browsedURL: "https://github.com/jobs/10?check_suite_focus=true", + wantOut: "Opening github.com/jobs/10 in your browser.\n", }, { name: "hide job header, failure", @@ -788,7 +788,7 @@ func TestViewRun(t *testing.T) { httpmock.REST("GET", "repos/OWNER/REPO/actions/runs/123/artifacts"), httpmock.StringResponse(`{}`)) }, - wantOut: "\nX trunk failed no job · 123\nTriggered via push about 59 minutes ago\n\nX This run likely failed because of a workflow file issue.\n\nFor more information, see: runs/123\n", + wantOut: "\nX trunk failed no job · 123\nTriggered via push about 59 minutes ago\n\nX This run likely failed because of a workflow file issue.\n\nFor more information, see: https://github.com/runs/123\n", }, { name: "hide job header, startup_failure", @@ -807,7 +807,7 @@ func TestViewRun(t *testing.T) { httpmock.REST("GET", "repos/OWNER/REPO/actions/runs/123/artifacts"), httpmock.StringResponse(`{}`)) }, - wantOut: "\nX trunk failed no job · 123\nTriggered via push about 59 minutes ago\n\nX This run likely failed because of a workflow file issue.\n\nFor more information, see: runs/123\n", + wantOut: "\nX trunk failed no job · 123\nTriggered via push about 59 minutes ago\n\nX This run likely failed because of a workflow file issue.\n\nFor more information, see: https://github.com/runs/123\n", }, } diff --git a/pkg/cmd/secret/list/list.go b/pkg/cmd/secret/list/list.go index d331482b8..e8694c62c 100644 --- a/pkg/cmd/secret/list/list.go +++ b/pkg/cmd/secret/list/list.go @@ -3,7 +3,6 @@ package list import ( "fmt" "net/http" - "net/url" "strings" "time" @@ -160,14 +159,8 @@ func getOrgSecrets(client *api.Client, host, orgName string) ([]*Secret, error) if secret.SelectedReposURL == "" { continue } - u, err := url.Parse(secret.SelectedReposURL) - if err != nil { - return nil, fmt.Errorf("failed determining selected repositories for %s: %w", secret.Name, err) - } - var result responseData - err = client.REST(u.Host, "GET", u.Path[1:], nil, &result) - if err != nil { + if err := client.REST(host, "GET", secret.SelectedReposURL, nil, &result); err != nil { return nil, fmt.Errorf("failed determining selected repositories for %s: %w", secret.Name, err) } secret.NumSelectedRepos = result.TotalCount