STACKITGit/cli
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Fix requesting REST sub-resources on GHE

Browse source 
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.
This commit is contained in:
Mislav Marohnić 2021-04-19 12:08:57 +02:00
parent 09b09810dd
commit ac348b0dec
6 changed files with 49 additions and 36 deletions
Download patch file Download diff file Expand all files Collapse all files

10
api/client.go
View file

@ -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

20
api/client_test.go
View file

@ -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))

8
pkg/cmd/run/shared/shared.go
View file

@ -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

8
pkg/cmd/run/shared/test.go
View file

@ -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{
{

30
pkg/cmd/run/view/view_test.go
View file

@ -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=<job-id>\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=<job-id>\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=<job-id>
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=<job-id>\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=<job-id>\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=<job-id>\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=<job-id>\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=<job-id>\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=<job-id>\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",
},
}

9
pkg/cmd/secret/list/list.go
View file

@ -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