From d431ed61c4483d4f04c3062c92dd7f8e2c1642c5 Mon Sep 17 00:00:00 2001 From: Arda Kuyumcu Date: Thu, 27 Jan 2022 14:41:51 -0800 Subject: [PATCH 1/3] Add checks field to branch protection required status checks --- github/github-accessors.go | 16 ++++++++++ github/github-accessors_test.go | 20 ++++++++++++ github/repos.go | 23 +++++++++++--- github/repos_test.go | 54 +++++++++++++++++++++++++++++++-- 4 files changed, 106 insertions(+), 7 deletions(-) diff --git a/github/github-accessors.go b/github/github-accessors.go index b16aefff8dd..64507d088d6 100644 --- a/github/github-accessors.go +++ b/github/github-accessors.go @@ -1644,6 +1644,14 @@ func (b *BranchProtectionRuleEvent) GetSender() *User { return b.Sender } +// GetAppID returns the AppID field if it's non-nil, zero value otherwise. +func (c *Check) GetAppID() int { + if c == nil || c.AppID == nil { + return 0 + } + return *c.AppID +} + // GetApp returns the App field. func (c *CheckRun) GetApp() *App { if c == nil { @@ -15444,6 +15452,14 @@ func (r *RequiredReviewer) GetType() string { return *r.Type } +// GetChecks returns the Checks field if it's non-nil, zero value otherwise. +func (r *RequiredStatusChecksRequest) GetChecks() []Check { + if r == nil || r.Checks == nil { + return nil + } + return *r.Checks +} + // GetStrict returns the Strict field if it's non-nil, zero value otherwise. func (r *RequiredStatusChecksRequest) GetStrict() bool { if r == nil || r.Strict == nil { diff --git a/github/github-accessors_test.go b/github/github-accessors_test.go index 280dc0c7743..0ce3a77d130 100644 --- a/github/github-accessors_test.go +++ b/github/github-accessors_test.go @@ -1963,6 +1963,16 @@ func TestBranchProtectionRuleEvent_GetSender(tt *testing.T) { b.GetSender() } +func TestCheck_GetAppID(tt *testing.T) { + var zeroValue int + c := &Check{AppID: &zeroValue} + c.GetAppID() + c = &Check{} + c.GetAppID() + c = nil + c.GetAppID() +} + func TestCheckRun_GetApp(tt *testing.T) { c := &CheckRun{} c.GetApp() @@ -18043,6 +18053,16 @@ func TestRequiredReviewer_GetType(tt *testing.T) { r.GetType() } +func TestRequiredStatusChecksRequest_GetChecks(tt *testing.T) { + var zeroValue []Check + r := &RequiredStatusChecksRequest{Checks: &zeroValue} + r.GetChecks() + r = &RequiredStatusChecksRequest{} + r.GetChecks() + r = nil + r.GetChecks() +} + func TestRequiredStatusChecksRequest_GetStrict(tt *testing.T) { var zeroValue bool r := &RequiredStatusChecksRequest{Strict: &zeroValue} diff --git a/github/repos.go b/github/repos.go index a1c8c81099a..8bac85152b5 100644 --- a/github/repos.go +++ b/github/repos.go @@ -850,7 +850,7 @@ type AuthorizedActorNames struct { From []string `json:"from,omitempty"` } -// AuthorizedActorsOnly represents if the branche rule can be edited by authorized actors only. +// AuthorizedActorsOnly represents if the branch rule can be edited by authorized actors only. type AuthorizedActorsOnly struct { From *bool `json:"from,omitempty"` } @@ -872,19 +872,34 @@ type ProtectionRequest struct { RequiredConversationResolution *bool `json:"required_conversation_resolution,omitempty"` } -// RequiredStatusChecks represents the protection status of a individual branch. +// Check represents a status check to require in order to merge into an individual branch. +type Check struct { + // The name of the required check. (Required.) + Context string `json:"context"` + // The ID of the GitHub App that must provide this check. Omit this field to automatically select the GitHub App + // that has recently provided this check, or any app if it was not set by a GitHub App. Pass -1 to explicitly + // allow any app to set the status. + AppID *int `json:"app_id"` +} + +// RequiredStatusChecks represents the protection status of an individual branch. type RequiredStatusChecks struct { // Require branches to be up to date before merging. (Required.) Strict bool `json:"strict"` - // The list of status checks to require in order to merge into this - // branch. (Required; use []string{} instead of nil for empty list.) + // Deprecated: The list of status checks to require in order to merge into this branch. If any of these checks have + // recently been set by a particular GitHub App, they will be required to come from that app in future for the + // branch to merge. Use checks instead of contexts for more fine-grained control. + // (Required; use []string{} instead of nil for empty list.) Contexts []string `json:"contexts"` + // The list of status checks to require in order to merge into this branch. + Checks []Check `json:"checks"` } // RequiredStatusChecksRequest represents a request to edit a protected branch's status checks. type RequiredStatusChecksRequest struct { Strict *bool `json:"strict,omitempty"` Contexts []string `json:"contexts,omitempty"` + Checks *[]Check `json:"checks,omitempty"` } // PullRequestReviewsEnforcement represents the pull request reviews enforcement of a protected branch. diff --git a/github/repos_test.go b/github/repos_test.go index 58c8aab5784..f9cc0c4cfe8 100644 --- a/github/repos_test.go +++ b/github/repos_test.go @@ -1040,7 +1040,17 @@ func TestRepositoriesService_GetBranchProtection(t *testing.T) { fmt.Fprintf(w, `{ "required_status_checks":{ "strict":true, - "contexts":["continuous-integration"] + "contexts":["continuous-integration", "ci-with-app-id"], + "checks": [ + { + "context": "continuous-integration", + "app_id": null + }, + { + "context": "ci-with-app-id", + "app_id": 123 + } + ] }, "required_pull_request_reviews":{ "dismissal_restrictions":{ @@ -1080,7 +1090,16 @@ func TestRepositoriesService_GetBranchProtection(t *testing.T) { want := &Protection{ RequiredStatusChecks: &RequiredStatusChecks{ Strict: true, - Contexts: []string{"continuous-integration"}, + Contexts: []string{"continuous-integration", "ci-with-app-id"}, + Checks: []Check{ + { + Context: "continuous-integration", + }, + { + Context: "ci-with-app-id", + AppID: Int(123), + }, + }, }, RequiredPullRequestReviews: &PullRequestReviewsEnforcement{ DismissStaleReviews: true, @@ -1228,6 +1247,16 @@ func TestRepositoriesService_UpdateBranchProtection(t *testing.T) { RequiredStatusChecks: &RequiredStatusChecks{ Strict: true, Contexts: []string{"continuous-integration"}, + Checks: []Check{ + { + Context: "continuous-integration", + AppID: Int(-1), + }, + { + Context: "ci-with-app-id", + AppID: Int(123), + }, + }, }, RequiredPullRequestReviews: &PullRequestReviewsEnforcementRequest{ DismissStaleReviews: true, @@ -1257,7 +1286,17 @@ func TestRepositoriesService_UpdateBranchProtection(t *testing.T) { fmt.Fprintf(w, `{ "required_status_checks":{ "strict":true, - "contexts":["continuous-integration"] + "contexts":["continuous-integration"], + "checks": [ + { + "context": "continuous-integration", + "app_id": null + }, + { + "context": "ci-with-app-id", + "app_id": 123 + } + ] }, "required_pull_request_reviews":{ "dismissal_restrictions":{ @@ -1291,6 +1330,15 @@ func TestRepositoriesService_UpdateBranchProtection(t *testing.T) { RequiredStatusChecks: &RequiredStatusChecks{ Strict: true, Contexts: []string{"continuous-integration"}, + Checks: []Check{ + { + Context: "continuous-integration", + }, + { + Context: "ci-with-app-id", + AppID: Int(123), + }, + }, }, RequiredPullRequestReviews: &PullRequestReviewsEnforcement{ DismissStaleReviews: true, From 0ce707ee864289a18ae8dc40a1d0ea3e51eb8034 Mon Sep 17 00:00:00 2001 From: Arda Kuyumcu Date: Thu, 27 Jan 2022 14:55:28 -0800 Subject: [PATCH 2/3] Add new test, update field --- github/repos.go | 2 +- github/repos_test.go | 126 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 1 deletion(-) diff --git a/github/repos.go b/github/repos.go index 8bac85152b5..21ee59c4f76 100644 --- a/github/repos.go +++ b/github/repos.go @@ -892,7 +892,7 @@ type RequiredStatusChecks struct { // (Required; use []string{} instead of nil for empty list.) Contexts []string `json:"contexts"` // The list of status checks to require in order to merge into this branch. - Checks []Check `json:"checks"` + Checks *[]Check `json:"checks"` } // RequiredStatusChecksRequest represents a request to edit a protected branch's status checks. diff --git a/github/repos_test.go b/github/repos_test.go index f9cc0c4cfe8..470192668f7 100644 --- a/github/repos_test.go +++ b/github/repos_test.go @@ -1383,6 +1383,132 @@ func TestRepositoriesService_UpdateBranchProtection(t *testing.T) { }) } +func TestRepositoriesService_UpdateBranchProtection_NoRequiredChecks(t *testing.T) { + client, mux, _, teardown := setup() + defer teardown() + + input := &ProtectionRequest{ + RequiredStatusChecks: &RequiredStatusChecks{ + Strict: true, + Contexts: []string{"continuous-integration"}, + }, + RequiredPullRequestReviews: &PullRequestReviewsEnforcementRequest{ + DismissStaleReviews: true, + DismissalRestrictionsRequest: &DismissalRestrictionsRequest{ + Users: &[]string{"uu"}, + Teams: &[]string{"tt"}, + }, + }, + Restrictions: &BranchRestrictionsRequest{ + Users: []string{"u"}, + Teams: []string{"t"}, + Apps: []string{"a"}, + }, + } + + mux.HandleFunc("/repos/o/r/branches/b/protection", func(w http.ResponseWriter, r *http.Request) { + v := new(ProtectionRequest) + json.NewDecoder(r.Body).Decode(v) + + testMethod(t, r, "PUT") + if !cmp.Equal(v, input) { + t.Errorf("Request body = %+v, want %+v", v, input) + } + + // TODO: remove custom Accept header when this API fully launches + testHeader(t, r, "Accept", mediaTypeRequiredApprovingReviewsPreview) + fmt.Fprintf(w, `{ + "required_status_checks":{ + "strict":true, + "contexts":["continuous-integration"], + "checks": [ + { + "context": "continuous-integration", + "app_id": null + } + ] + }, + "required_pull_request_reviews":{ + "dismissal_restrictions":{ + "users":[{ + "id":3, + "login":"uu" + }], + "teams":[{ + "id":4, + "slug":"tt" + }] + }, + "dismiss_stale_reviews":true, + "require_code_owner_reviews":true + }, + "restrictions":{ + "users":[{"id":1,"login":"u"}], + "teams":[{"id":2,"slug":"t"}], + "apps":[{"id":3,"slug":"a"}] + } + }`) + }) + + ctx := context.Background() + protection, _, err := client.Repositories.UpdateBranchProtection(ctx, "o", "r", "b", input) + if err != nil { + t.Errorf("Repositories.UpdateBranchProtection returned error: %v", err) + } + + want := &Protection{ + RequiredStatusChecks: &RequiredStatusChecks{ + Strict: true, + Contexts: []string{"continuous-integration"}, + Checks: []Check{ + { + Context: "continuous-integration", + }, + }, + }, + RequiredPullRequestReviews: &PullRequestReviewsEnforcement{ + DismissStaleReviews: true, + DismissalRestrictions: &DismissalRestrictions{ + Users: []*User{ + {Login: String("uu"), ID: Int64(3)}, + }, + Teams: []*Team{ + {Slug: String("tt"), ID: Int64(4)}, + }, + }, + RequireCodeOwnerReviews: true, + }, + Restrictions: &BranchRestrictions{ + Users: []*User{ + {Login: String("u"), ID: Int64(1)}, + }, + Teams: []*Team{ + {Slug: String("t"), ID: Int64(2)}, + }, + Apps: []*App{ + {Slug: String("a"), ID: Int64(3)}, + }, + }, + } + if !cmp.Equal(protection, want) { + t.Errorf("Repositories.UpdateBranchProtection returned %+v, want %+v", protection, want) + } + + const methodName = "UpdateBranchProtection" + testBadOptions(t, methodName, func() (err error) { + _, _, err = client.Repositories.UpdateBranchProtection(ctx, "\n", "\n", "\n", input) + return err + }) + + testNewRequestAndDoFailure(t, methodName, client, func() (*Response, error) { + got, resp, err := client.Repositories.UpdateBranchProtection(ctx, "o", "r", "b", input) + if got != nil { + t.Errorf("testNewRequestAndDoFailure %v = %#v, want nil", methodName, got) + } + return resp, err + }) +} + func TestRepositoriesService_RemoveBranchProtection(t *testing.T) { client, mux, _, teardown := setup() defer teardown() From 0a3981ad2d2834e156398232e145c7b8024e452a Mon Sep 17 00:00:00 2001 From: Arda Kuyumcu Date: Thu, 27 Jan 2022 15:10:45 -0800 Subject: [PATCH 3/3] Run go generate --- github/github-accessors.go | 8 ++++++++ github/github-accessors_test.go | 10 ++++++++++ 2 files changed, 18 insertions(+) diff --git a/github/github-accessors.go b/github/github-accessors.go index 64507d088d6..646dd1fb968 100644 --- a/github/github-accessors.go +++ b/github/github-accessors.go @@ -15452,6 +15452,14 @@ func (r *RequiredReviewer) GetType() string { return *r.Type } +// GetChecks returns the Checks field if it's non-nil, zero value otherwise. +func (r *RequiredStatusChecks) GetChecks() []Check { + if r == nil || r.Checks == nil { + return nil + } + return *r.Checks +} + // GetChecks returns the Checks field if it's non-nil, zero value otherwise. func (r *RequiredStatusChecksRequest) GetChecks() []Check { if r == nil || r.Checks == nil { diff --git a/github/github-accessors_test.go b/github/github-accessors_test.go index 0ce3a77d130..e3f0caf855f 100644 --- a/github/github-accessors_test.go +++ b/github/github-accessors_test.go @@ -18053,6 +18053,16 @@ func TestRequiredReviewer_GetType(tt *testing.T) { r.GetType() } +func TestRequiredStatusChecks_GetChecks(tt *testing.T) { + var zeroValue []Check + r := &RequiredStatusChecks{Checks: &zeroValue} + r.GetChecks() + r = &RequiredStatusChecks{} + r.GetChecks() + r = nil + r.GetChecks() +} + func TestRequiredStatusChecksRequest_GetChecks(tt *testing.T) { var zeroValue []Check r := &RequiredStatusChecksRequest{Checks: &zeroValue}