Required signatures on protected branches #1039

dlnilsson · 2018-10-29T15:17:07Z

Added support for the preview required commit signatures for Protected Branches API.

https://developer.github.com/changes/2018-02-22-protected-branches-required-signatures/

This solves: #902

Preview required commit signatures for Protected Branches API https://developer.github.com/changes/2018-02-22-protected-branches-required-signatures/ Signed-off-by: Daniel Nilsson <[email protected]>

Signed-off-by: Daniel Nilsson <[email protected]>

gmlewis

Thank you, @danijeel!
I have a few requested changes and a couple open questions.

gmlewis · 2018-10-30T01:14:19Z

github/repos.go

@@ -698,6 +698,13 @@ type DismissalRestrictionsRequest struct {
 	Teams *[]string `json:"teams,omitempty"`
 }

+// SignaturesProtectedBranch represents the protection status of a individual branch.


s/a individual/an individual/

gmlewis · 2018-10-30T01:16:59Z

github/repos.go

+type SignaturesProtectedBranch struct {
+	URL *string `json:"url,omitempty"`
+	// Commits pushed to matching branches must have verified signatures.
+	Enabled bool `json:"strict"`


Why not

Strict *bool `json:"strict,omitempty"`

?

gmlewis · 2018-10-30T01:18:18Z

github/repos.go

@@ -850,6 +857,67 @@ func (s *RepositoriesService) RemoveBranchProtection(ctx context.Context, owner,
 	return s.client.Do(ctx, req, nil)
 }

+// GetSignaturesProtectedBranch get required signatures of protected branch


s/get /gets /
s/branch/branch./

gmlewis · 2018-11-02T01:26:43Z

github/repos.go

+	return p, resp, nil
+}
+
+// AddSignatureProtectedBranch Require signed commits to a protected branch.


I'm wondering if this should be renamed... I'm not even happy with the working on the GitHub API docs themselves.
From the description at the provided URL, it sounds like this endpoint is used to require signatures on protected branches. So how about this:

`// RequireSignaturesOnProtectedBranch makes signed commits required on a protected branch.```

gmlewis · 2018-11-02T01:28:24Z

github/repos.go

+	return r, resp, err
+}
+
+// RemoveSignatureProtectedBranch removes required signed commits on a given branch.


Maybe OptionalSignaturesOnProtectedBranch ?

Signed-off-by: Daniel Nilsson <[email protected]>

dlnilsson · 2018-11-02T07:28:33Z

Thank you for the review and feedback @gmlewis! I've updated the comments you mention above.

gmlewis

Thank you, @danijeel!
One more small tweak and then LGTM.
Awaiting second LGTM before merging.

gmlewis · 2018-11-02T10:47:09Z

github/repos.go

 type SignaturesProtectedBranch struct {
 	URL *string `json:"url,omitempty"`
 	// Commits pushed to matching branches must have verified signatures.
-	Enabled bool `json:"strict"`
+	Enabled bool `json:"strict,omitempty"`


This still needs *bool.

Signed-off-by: Daniel Nilsson <[email protected]>

dlnilsson · 2018-11-02T11:25:27Z

Ah, yes. thank you @gmlewis. Could you please review the test cases if they make sense?

on SignaturesProtectedBranch Signed-off-by: Daniel Nilsson <[email protected]>

Signed-off-by: Daniel Nilsson <[email protected]>

gauntface

LGTM thanks for your contribution 👍

gmlewis

Just a few minor changes, please.
Thank you, @danijeel!

gmlewis · 2018-11-03T00:09:52Z

github/repos_test.go

@@ -1066,13 +1066,13 @@ func TestRepositoriesService_GetSignaturesProtectedBranch(t *testing.T) {
 	})

 	signature, _, err := client.Repositories.GetSignaturesProtectedBranch(context.Background(), "o", "r", "b")
+


Please delete this blank line, as we attempt to keep the error checking code tightly bound to the section of code that created it.

gmlewis · 2018-11-03T00:12:03Z

github/repos_test.go

 	if err != nil {
 		t.Errorf("Repositories.GetSignaturesProtectedBranch returned error: %v", err)
 	}

 	want := &SignaturesProtectedBranch{
-		URL:     String("/repos/o/r/branches/b/protection/required_signatures"),
-		Enabled: false,
+		URL: String("/repos/o/r/branches/b/protection/required_signatures"),


Please add back your Enabled: Bool(false), line here and below.

gmlewis · 2018-11-03T00:12:16Z

github/repos_test.go

@@ -1096,9 +1096,9 @@ func TestRepositoriesService_AddSignatureProtectedBranch(t *testing.T) {
 	}

 	want := &SignaturesProtectedBranch{
-		URL:     String("/repos/o/r/branches/b/protection/required_signatures"),
-		Enabled: false,
+		URL: String("/repos/o/r/branches/b/protection/required_signatures"),


Please add back your Enabled: Bool(false), line here.

Signed-off-by: Daniel Nilsson <[email protected]>

gmlewis · 2018-11-08T03:47:10Z

github/repos_test.go

+	}
+}
+
+func TestRepositoriesService_RemoveSignatureProtectedBranch(t *testing.T) {


s/RemoveSignature/OptionalSignaturesOn/

gmlewis · 2018-11-08T03:50:21Z

github/repos.go

+type SignaturesProtectedBranch struct {
+	URL *string `json:"url,omitempty"`
+	// Commits pushed to matching branches must have verified signatures.
+	Enabled *bool `json:"strict,omitempty"`


s/strict/enabled/

Signed-off-by: Daniel Nilsson <[email protected]>

gmlewis

Thank you, @danijeel and @gauntface!
LGTM.
Merging since we already have second LGTM.

Fixes google#902.

dlnilsson added 2 commits October 29, 2018 16:06

Added support for github api preview signed commits.

dfdbc92

Preview required commit signatures for Protected Branches API https://developer.github.com/changes/2018-02-22-protected-branches-required-signatures/ Signed-off-by: Daniel Nilsson <[email protected]>

added SignaturesProtectedBranch getURL

c5c33fc

Signed-off-by: Daniel Nilsson <[email protected]>

googlebot added the cla: yes Indication that the PR author has signed a Google Contributor License Agreement. label Oct 29, 2018

gmlewis requested changes Nov 2, 2018

View reviewed changes

gmlewis added the question label Nov 2, 2018

Corrected grammar and naming convention.

00f96c6

Signed-off-by: Daniel Nilsson <[email protected]>

gmlewis requested changes Nov 2, 2018

View reviewed changes

gmlewis added NeedsReview PR is awaiting a review before merging. and removed question labels Nov 2, 2018

gmlewis requested a review from gauntface November 2, 2018 10:52

Added pointer for Enabled bool.

302d642

Signed-off-by: Daniel Nilsson <[email protected]>

dlnilsson added 2 commits November 2, 2018 13:59

added github-accessor for GetEnabled()

ae38ea7

on SignaturesProtectedBranch Signed-off-by: Daniel Nilsson <[email protected]>

go fmt

d697b01

Signed-off-by: Daniel Nilsson <[email protected]>

gauntface approved these changes Nov 2, 2018

View reviewed changes

gmlewis requested changes Nov 3, 2018

View reviewed changes

re-added assertion of Enabled

b521515

Signed-off-by: Daniel Nilsson <[email protected]>

gmlewis reviewed Nov 8, 2018

View reviewed changes

dlnilsson added 2 commits November 8, 2018 14:36

rename test func name to reflect refactor.

9d0cbac

Signed-off-by: Daniel Nilsson <[email protected]>

keep Enabled json key if empty.

6b75719

Signed-off-by: Daniel Nilsson <[email protected]>

gmlewis approved these changes Nov 9, 2018

View reviewed changes

gmlewis merged commit 99760a1 into google:master Nov 9, 2018

dlnilsson deleted the required_signatures branch January 3, 2019 18:36

n1lesh pushed a commit to n1lesh/go-github that referenced this pull request Oct 2, 2020

Required signatures on protected branches (google#1039)

1155396

Fixes google#902.

gmlewis removed the NeedsReview PR is awaiting a review before merging. label Mar 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Required signatures on protected branches #1039

Required signatures on protected branches #1039

dlnilsson commented Oct 29, 2018

gmlewis left a comment

gmlewis Oct 30, 2018

gmlewis Oct 30, 2018

gmlewis Oct 30, 2018

gmlewis Nov 2, 2018

gmlewis Nov 2, 2018

dlnilsson commented Nov 2, 2018

gmlewis left a comment

gmlewis Nov 2, 2018

dlnilsson commented Nov 2, 2018

gauntface left a comment

gmlewis left a comment

gmlewis Nov 3, 2018

gmlewis Nov 3, 2018

gmlewis Nov 3, 2018

gmlewis Nov 8, 2018

gmlewis Nov 8, 2018

gmlewis left a comment

		@@ -1066,13 +1066,13 @@ func TestRepositoriesService_GetSignaturesProtectedBranch(t *testing.T) {
		})

		signature, _, err := client.Repositories.GetSignaturesProtectedBranch(context.Background(), "o", "r", "b")

Required signatures on protected branches #1039

Required signatures on protected branches #1039

Conversation

dlnilsson commented Oct 29, 2018

gmlewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlnilsson commented Nov 2, 2018

gmlewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dlnilsson commented Nov 2, 2018

gauntface left a comment

Choose a reason for hiding this comment

gmlewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmlewis left a comment

Choose a reason for hiding this comment