Error codes for failed repo webhook requests are "Unknown"

[jusunglee]

Repositories.ListHooks(ctx, owner, repository, opts) returns hooks, response, and error. If response failed with codes such as 404 or 403, this is not reflected in the error code. Using errors.Code(err) when response returns an error HTTP status code will simply return codes.Unknown, when it should really be codes.PermissionDenied or codes.NotFound.

Propose a change where we map HTTP Response status codes (if error) into the error object's error code.

[gmlewis]

Thank you, @jusunglee ! I apologize for missing this issue 3 (!) years ago!!!

This would be a great PR for any new contributor to this repo or a new Go developer.
All contributions are greatly appreciated!

Feel free to volunteer for any issue and the issue can be assigned to you so that others don't attempt to duplicate the work.

Please check out our CONTRIBUTING.md guide to get started. (In particular, please remember to go generate ./... and don't use force-push to your PRs.)

Thank you!

[AGMETEOR]

@gmlewis can I take a look?

[gmlewis]

Sure, @AGMETEOR ... It's yours.

[AGMETEOR]

Still on it. Will get some time this Friday to fix.

[AGMETEOR]

Hi @gmlewis , is it possible to rephrase what exactly we should implement for this issue based on the current state of the code?

[gmlewis]

Hi @gmlewis , is it possible to rephrase what exactly we should implement for this issue based on the current state of the code?

Yes. The Repositories.ListHooks(ctx, owner, repository, opts) endpoint needs to take a look at any error codes it receives within the response itself. Note that this does not mean that err != nil... this means that the resp.StatusCode will equal either http.StatusForbidden (403) or http.StatusNotFound (404). (See: https://pkg.go.dev/net/http#pkg-constants).

If either of these codes are returned within resp.StatusCode, then we want to make sure these status codes come through.
You will want to look at the CheckResponse method and make sure that the error is getting through properly. Maybe it is since this ticket is 3 years old... so maybe all this will require is adding another unit test for the Repositories.ListHooks(ctx, owner, repository, opts) endpoint and verify that these two errors are getting returned properly. (I'm not sure what the original description means by codes.Unknown... I couldn't find that anywhere... so that is probably safe to ignore.)

If they are, the PR will only have the added unit test.

If they are not, then let's try and figure out how to make these status codes visible to the client user of the library.

Thanks, @AGMETEOR !

[AGMETEOR]

Thanks @gmlewis . Been out on leave so this got delayed a bit. But will start working on it before this week ends.

[AGMETEOR]

Hi @gmlewis , from looking at the code looks like the they're being returned properly. I wrote a quick test like so, thoughts?

func testErrorStatusCode(t *testing.T, code int) {
	client, mux, _, teardown := setup()
	defer teardown()

	mux.HandleFunc("/repos/o/r/hooks", func(w http.ResponseWriter, r *http.Request) {
		testMethod(t, r, "GET")
		w.WriteHeader(code)
	})

	ctx := context.Background()
	_, _, err := client.Repositories.ListHooks(ctx, "o", "r", nil)

	v, ok := err.(*ErrorResponse)
	if !ok {
		t.Error("Returned error not of Error Response type")
	}

	if code != v.Response.StatusCode {
		t.Error("Error response does not contain status code")
	}
}

func TestRepositoriesService_ListHooks_403_code_no_rate_limit(t *testing.T) {
	testErrorStatusCode(t, http.StatusForbidden)
}

func TestRepositoriesService_ListHooks_404_code(t *testing.T) {
	testErrorStatusCode(t, http.StatusNotFound)
}

[gmlewis]

Excellent, @AGMETEOR !
Thank you for verifying that the codes are coming through properly.

If you would like to add a PR demonstrating this, that's great... otherwise if you just want to have this documented in this issue that is totally fine with me too and we can close it. Your call.

[AGMETEOR]

@gmlewis i prefer to push a PR with the test. I’ll do that later today