Repository: google/go-github
Pull request #2827 (2023-07-05 11:27:15)

Add support for personal access tokens request review API
Following up with my previous PR #2826, I am also proposing the following changes, which add support for [programmatically approving or denying](https://docs.github.com/en/rest/orgs/personal-access-tokens?apiVersion=2022-11-28#review-a-request-to-access-organization-resources-with-a-fine-grained-personal-access-token) fine-grained personal access token requests.

I thought about including the rest of this API, but I don't have an immediate need for it and the PR might get a little too big. I was also a bit unsure on the function signature, so let me know your thoughts.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/google/go-github/pull/2827?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google) Report
> Merging [#2827](https://app.codecov.io/gh/google/go-github/pull/2827?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google) (7ebd698) into [master](https://app.codecov.io/gh/google/go-github/commit/9f7124cc0903ad531ba469d2f1403fb3e39645cf?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google) (9f7124c) will **increase** coverage by `0.00%`.
> The diff coverage is `100.00%`.

```diff
@@           Coverage Diff           @@
##           master    #2827   +/-   ##
=======================================
  Coverage   98.06%   98.06%           
=======================================
  Files         136      137    +1     
  Lines       12279    12294   +15     
=======================================
+ Hits        12041    12056   +15     
  Misses        162      162           
  Partials       76       76           
```


| [Impacted Files](https://app.codecov.io/gh/google/go-github/pull/2827?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google) | Coverage Δ | |
|---|---|---|
| [github/orgs\_personal\_access\_tokens.go](https://app.codecov.io/gh/google/go-github/pull/2827?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=google#diff-Z2l0aHViL29yZ3NfcGVyc29uYWxfYWNjZXNzX3Rva2Vucy5nbw==) | `100.00% <100.00%> (ø)` | |


joaopenteado: @gmlewis thanks again for the quick feedback! I'll probably commit the requested changes by tomorrow.

> one of the reasons for this is that GitHub has a history of adding new options to API endpoints. If we didn't do this, then we would be continually breaking the API of this client library by being forced to update the signature of this method to keep adding new fields.

This makes perfect sense. I should've thought of that, thanks for the heads up!

Just two questions though, as I am still not very familiar with this package's approach to API design. I saw a lot of other methods using pointers to the `Options` struct containing the body payload. This is good in the sense that it could potentially avoid a copy of a big struct, but it also allows for passing in a `nil` pointer which could then likely be dereferenced later on and cause a `panic` or a runtime error in case a payload is required. I take it this is an accepted risk in this package's design?

Additionally, the `Options` structs themselves will often (_but strangely not always_) be composed of only reference types. I imagine this is done intentionally in order to distinguish between Go's default initialized values and the presence or absence of values in the option itself. However, this also could introduce a runtime panic if a required parameter is forgotten, is this also by design then?

In order words, despite `Action` being mandatory for this specify endpoint, I should design my `Options` struct like this:
```go
type ReviewPersonalAccessTokenRequestOptions struct {
	Action *string `json:"action,omitempty"`
	Reason *string `json:"reason,omitempty"`
}
```
And not like this:
```go
type ReviewPersonalAccessTokenRequestOptions struct {
	Action string `json:"action"`
	Reason *string `json:"reason,omitempty"` // or string
}
```

Is my understanding of the above points, correct?
gmlewis: > ... I saw a lot of other methods using pointers to the Options struct containing the body payload. ... I take it this is an accepted risk in this package's design?

So you will probably see inconsistencies in this regard within this client library, which I will take the blame for.
Ideally, if a method requires body parameters to be provided, then we would simply pass an `opts` parameter that is a struct passed by value (and not a struct pointer), and what I would recommend in this case.

> Additionally, the Options structs themselves will often (but strangely not always) be composed of only reference types. ...  is this also by design then?

Again, ideally, if a passed parameter is mandatory, we would not use a pointer, and we would not use `omitempty` in the JSON tag.  If a passed parameter is optional, then we use a pointer and include `omitempty`. There are also a few fiddly cases in the API where we must send a `null` or other weirdness where we take other measures, but I don't think this particular endpoint has any fiddly cases.

So it appears to me that your last suggestion would be preferred since `action` is mandatory and `reason` is not:

```go
type ReviewPersonalAccessTokenRequestOptions struct {
	Action string `json:"action"`
	Reason *string `json:"reason,omitempty"`
}
```
gmlewis: In case the last comment wasn't clear, I would think this method would have this in it:
`opts ReviewPersonalAccessTokenRequestOptions) ...`

(in other words, _not_ a pointer to a struct, but instead using pass-by-value.)
joaopenteado: @gmlewis Thank you for clarifying!
I've pushed some changes, let me know if you'd like me to change anything else.
gmlewis: > Do you think I should keep it as a `string` just in case GitHub changes their mind about this ID later, or switch it to a `int64`?

Since the official docs say that it is an "integer", then let's please make it an `int64` in the hopes that that will reduce developer confusion. Thanks.
gmlewis: Oh, and you can leave all the "%v" in the `fmt.Sprintf` as that is idiomatic Go and commonly used throughout this repo.  We only change this "%v" to other things when formatting must be non-standard (e.g. `"%05d"`).
joaopenteado: Done!
gmlewis: Thank you, @liaodaniel !
Merging.

Patch:
diff --git a/github/github-accessors.go b/github/github-accessors.go
index 41cd582c..c7089951 100644
--- a/github/github-accessors.go
+++ b/github/github-accessors.go
@@ -18806,6 +18806,14 @@ func (r *ReviewersRequest) GetNodeID() string {
 	return *r.NodeID
 }
 
+// GetReason returns the Reason field if it's non-nil, zero value otherwise.
+func (r *ReviewPersonalAccessTokenRequestOptions) GetReason() string {
+	if r == nil || r.Reason == nil {
+		return ""
+	}
+	return *r.Reason
+}
+
 // GetDescription returns the Description field if it's non-nil, zero value otherwise.
 func (r *Rule) GetDescription() string {
 	if r == nil || r.Description == nil {
diff --git a/github/orgs_personal_access_tokens.go b/github/orgs_personal_access_tokens.go
new file mode 100644
index 00000000..c30ff284
--- /dev/null
+++ b/github/orgs_personal_access_tokens.go
@@ -0,0 +1,34 @@
+// Copyright 2023 The go-github AUTHORS. All rights reserved.
+//
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package github
+
+import (
+	"context"
+	"fmt"
+	"net/http"
+)
+
+// ReviewPersonalAccessTokenRequestOptions specifies the parameters to the ReviewPersonalAccessTokenRequest method.
+type ReviewPersonalAccessTokenRequestOptions struct {
+	Action string  `json:"action"`
+	Reason *string `json:"reason,omitempty"`
+}
+
+// ReviewPersonalAccessTokenRequest approves or denies a pending request to access organization resources via a fine-grained personal access token.
+// Only GitHub Apps can call this API, using the `organization_personal_access_token_requests: write` permission.
+// `action` can be one of `approve` or `deny`.
+//
+// GitHub API docs: https://docs.github.com/en/rest/orgs/personal-access-tokens?apiVersion=2022-11-28#review-a-request-to-access-organization-resources-with-a-fine-grained-personal-access-token
+func (s *OrganizationsService) ReviewPersonalAccessTokenRequest(ctx context.Context, org string, requestID int64, opts ReviewPersonalAccessTokenRequestOptions) (*Response, error) {
+	u := fmt.Sprintf("orgs/%v/personal-access-token-requests/%v", org, requestID)
+
+	req, err := s.client.NewRequest(http.MethodPost, u, &opts)
+	if err != nil {
+		return nil, err
+	}
+
+	return s.client.Do(ctx, req, nil)
+}


Test patch:
diff --git a/github/github-accessors_test.go b/github/github-accessors_test.go
index 993a1248..7d151331 100644
--- a/github/github-accessors_test.go
+++ b/github/github-accessors_test.go
@@ -21933,6 +21933,16 @@ func TestReviewersRequest_GetNodeID(tt *testing.T) {
 	r.GetNodeID()
 }
 
+func TestReviewPersonalAccessTokenRequestOptions_GetReason(tt *testing.T) {
+	var zeroValue string
+	r := &ReviewPersonalAccessTokenRequestOptions{Reason: &zeroValue}
+	r.GetReason()
+	r = &ReviewPersonalAccessTokenRequestOptions{}
+	r.GetReason()
+	r = nil
+	r.GetReason()
+}
+
 func TestRule_GetDescription(tt *testing.T) {
 	var zeroValue string
 	r := &Rule{Description: &zeroValue}
diff --git a/github/orgs_personal_access_tokens_test.go b/github/orgs_personal_access_tokens_test.go
new file mode 100644
index 00000000..93ee1b3d
--- /dev/null
+++ b/github/orgs_personal_access_tokens_test.go
@@ -0,0 +1,73 @@
+// Copyright 2023 The go-github AUTHORS. All rights reserved.
+//
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package github
+
+import (
+	"context"
+	"encoding/json"
+	"net/http"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+)
+
+func TestOrganizationsService_ReviewPersonalAccessTokenRequest(t *testing.T) {
+	client, mux, _, teardown := setup()
+	defer teardown()
+
+	input := ReviewPersonalAccessTokenRequestOptions{
+		Action: "a",
+		Reason: String("r"),
+	}
+
+	mux.HandleFunc("/orgs/o/personal-access-token-requests/1", func(w http.ResponseWriter, r *http.Request) {
+		v := new(ReviewPersonalAccessTokenRequestOptions)
+		json.NewDecoder(r.Body).Decode(v)
+
+		testMethod(t, r, http.MethodPost)
+		if !cmp.Equal(v, &input) {
+			t.Errorf("Request body = %+v, want %+v", v, input)
+		}
+
+		w.WriteHeader(http.StatusNoContent)
+	})
+
+	ctx := context.Background()
+	res, err := client.Organizations.ReviewPersonalAccessTokenRequest(ctx, "o", 1, input)
+	if err != nil {
+		t.Errorf("Organizations.ReviewPersonalAccessTokenRequest returned error: %v", err)
+	}
+
+	if res.StatusCode != http.StatusNoContent {
+		t.Errorf("Organizations.ReviewPersonalAccessTokenRequest returned %v, want %v", res.StatusCode, http.StatusNoContent)
+	}
+
+	const methodName = "ReviewPersonalAccessTokenRequest"
+	testBadOptions(t, methodName, func() (err error) {
+		_, err = client.Organizations.ReviewPersonalAccessTokenRequest(ctx, "\n", 0, input)
+		return err
+	})
+
+	testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) {
+		return client.Organizations.ReviewPersonalAccessTokenRequest(ctx, "o", 1, input)
+	})
+}
+
+func TestReviewPersonalAccessTokenRequestOptions_Marshal(t *testing.T) {
+	testJSONMarshal(t, &ReviewPersonalAccessTokenRequestOptions{}, "{}")
+
+	u := &ReviewPersonalAccessTokenRequestOptions{
+		Action: "a",
+		Reason: String("r"),
+	}
+
+	want := `{
+		"action": "a",
+		"reason": "r"
+	}`
+
+	testJSONMarshal(t, u, want)
+}