Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add RequiredApprovingReviewCount to PullRequestReviewsEnforcement #880

Merged
merged 15 commits into from
Aug 10, 2018
Merged

Add RequiredApprovingReviewCount to PullRequestReviewsEnforcement #880

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
a49ec22
Added RequiredApprovingReviewCount to PullRequestReviewsEnforcement
victoryNap Mar 28, 2018
c51d8bb
Added tests
victoryNap Mar 28, 2018
d578d31
Fixed tests
victoryNap Mar 28, 2018
70d6dcf
Merge branch 'master' into master
victoryNap Mar 28, 2018
df5d65e
Added tags and made associated values pointers, also updated related…
victoryNap Mar 29, 2018
ef58a35
Updated documentation to specify when values are optional
victoryNap Mar 29, 2018
5af409a
Added a new header for RequiredApprovingReviewsPreview, replaced and …
victoryNap May 4, 2018
4e70bd3
Merge branch 'master' into master
victoryNap May 4, 2018
da53bef
Fixed formatting on repos_test.go
victoryNap May 4, 2018
2b2e379
Merge branch 'master' of https://github.com/victoryNap/go-github
victoryNap May 4, 2018
1cba8d0
Updated formatting on github.go
victoryNap May 4, 2018
485a5d5
Updated comment to reflect that new field is not optional
victoryNap May 11, 2018
09a561d
Fix indentation.
victoryNap Jun 1, 2018
0966f88
Removed unnecessary omitempty annotations.
victoryNap Jun 1, 2018
7aff404
Fixed tests to include new value
victoryNap Jun 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions github/github.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,6 @@ const (
// https://developer.github.com/changes/2014-12-09-new-attributes-for-stars-api/
mediaTypeStarringPreview = "application/vnd.github.v3.star+json"

// https://developer.github.com/changes/2015-11-11-protected-branches-api/
mediaTypeProtectedBranchesPreview = "application/vnd.github.loki-preview+json"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems odd that there wasn't a GitHub Developer API announcement revoking this custom media type. (Unless maybe I missed it?)
The last one I see was: https://developer.github.com/changes/2016-06-27-protected-branches-api-update/
and the new docs only mention the new custom media type, so maybe this one silently went away?
I'm guessing that's how you interpret the current situation, @victoryNap, judging from the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also didn't see an update saying it was no longer a preview, but thats the assumption that I made, their documentation makes no reference to the previous preview header, only to the new one that I've added.

screen shot 2018-05-10 at 7 58 52 pm


// https://help.github.com/enterprise/2.4/admin/guides/migrations/exporting-the-github-com-organization-s-repositories/
mediaTypeMigrationsPreview = "application/vnd.github.wyandotte-preview+json"

Expand Down Expand Up @@ -114,6 +111,9 @@ const (
// https://developer.github.com/changes/2018-01-25-organization-invitation-api-preview/
mediaTypeOrganizationInvitationPreview = "application/vnd.github.dazzler-preview+json"

// https://developer.github.com/changes/2018-03-16-protected-branches-required-approving-reviews/
mediaTypeRequiredApprovingReviewsPreview = "application/vnd.github.luke-cage-preview+json"

// https://developer.github.com/changes/2018-02-22-label-description-search-preview/
mediaTypeLabelDescriptionSearchPreview = "application/vnd.github.symmetra-preview+json"

Expand Down
37 changes: 23 additions & 14 deletions github/repos.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,6 +565,9 @@ type PullRequestReviewsEnforcement struct {
DismissStaleReviews bool `json:"dismiss_stale_reviews"`
// RequireCodeOwnerReviews specifies if an approved review is required in pull requests including files with a designated code owner.
RequireCodeOwnerReviews bool `json:"require_code_owner_reviews"`
// RequiredApprovingReviewCount specifies the number of approvals required before the pull request can be merged.
// Valid values are 1-6.
RequiredApprovingReviewCount int `json:"required_approving_review_count"`
}

// PullRequestReviewsEnforcementRequest represents request to set the pull request review
Expand All @@ -579,6 +582,9 @@ type PullRequestReviewsEnforcementRequest struct {
DismissStaleReviews bool `json:"dismiss_stale_reviews"`
// RequireCodeOwnerReviews specifies if an approved review is required in pull requests including files with a designated code owner.
RequireCodeOwnerReviews bool `json:"require_code_owner_reviews"`
// RequiredApprovingReviewCount specifies the number of approvals required before the pull request can be merged.
// Valid values are 1-6.
RequiredApprovingReviewCount int `json:"required_approving_review_count,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it can be omitted, then I think we need to make this type *int instead of int.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue with the comment, I forgot to update it. Its a required field.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then in that case, should we get rid of the ,omitempty here and below?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are your thoughts, @victoryNap on this ,omitempty?
I'm thinking it should be removed since it is a required parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call.

}

// PullRequestReviewsEnforcementUpdate represents request to patch the pull request review
Expand All @@ -591,6 +597,9 @@ type PullRequestReviewsEnforcementUpdate struct {
DismissStaleReviews *bool `json:"dismiss_stale_reviews,omitempty"`
// RequireCodeOwnerReviews specifies if an approved review is required in pull requests including files with a designated code owner.
RequireCodeOwnerReviews bool `json:"require_code_owner_reviews,omitempty"`
// RequiredApprovingReviewCount specifies the number of approvals required before the pull request can be merged.
// Valid values are 1 - 6.
RequiredApprovingReviewCount int `json:"required_approving_review_count,omitempty"`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here: *int.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as above

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and this one too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also Updated

}

// AdminEnforcement represents the configuration to enforce required status checks for repository administrators.
Expand Down Expand Up @@ -655,7 +664,7 @@ func (s *RepositoriesService) ListBranches(ctx context.Context, owner string, re
}

// TODO: remove custom Accept header when this API fully launches
req.Header.Set("Accept", mediaTypeProtectedBranchesPreview)
req.Header.Set("Accept", mediaTypeRequiredApprovingReviewsPreview)

var branches []*Branch
resp, err := s.client.Do(ctx, req, &branches)
Expand All @@ -677,7 +686,7 @@ func (s *RepositoriesService) GetBranch(ctx context.Context, owner, repo, branch
}

// TODO: remove custom Accept header when this API fully launches
req.Header.Set("Accept", mediaTypeProtectedBranchesPreview)
req.Header.Set("Accept", mediaTypeRequiredApprovingReviewsPreview)

b := new(Branch)
resp, err := s.client.Do(ctx, req, b)
Expand All @@ -699,7 +708,7 @@ func (s *RepositoriesService) GetBranchProtection(ctx context.Context, owner, re
}

// TODO: remove custom Accept header when this API fully launches
req.Header.Set("Accept", mediaTypeProtectedBranchesPreview)
req.Header.Set("Accept", mediaTypeRequiredApprovingReviewsPreview)

p := new(Protection)
resp, err := s.client.Do(ctx, req, p)
Expand All @@ -721,7 +730,7 @@ func (s *RepositoriesService) GetRequiredStatusChecks(ctx context.Context, owner
}

// TODO: remove custom Accept header when this API fully launches
req.Header.Set("Accept", mediaTypeProtectedBranchesPreview)
req.Header.Set("Accept", mediaTypeRequiredApprovingReviewsPreview)

p := new(RequiredStatusChecks)
resp, err := s.client.Do(ctx, req, p)
Expand All @@ -743,7 +752,7 @@ func (s *RepositoriesService) ListRequiredStatusChecksContexts(ctx context.Conte
}

// TODO: remove custom Accept header when this API fully launches
req.Header.Set("Accept", mediaTypeProtectedBranchesPreview)
req.Header.Set("Accept", mediaTypeRequiredApprovingReviewsPreview)

resp, err = s.client.Do(ctx, req, &contexts)
if err != nil {
Expand All @@ -764,7 +773,7 @@ func (s *RepositoriesService) UpdateBranchProtection(ctx context.Context, owner,
}

// TODO: remove custom Accept header when this API fully launches
req.Header.Set("Accept", mediaTypeProtectedBranchesPreview)
req.Header.Set("Accept", mediaTypeRequiredApprovingReviewsPreview)

p := new(Protection)
resp, err := s.client.Do(ctx, req, p)
Expand All @@ -786,7 +795,7 @@ func (s *RepositoriesService) RemoveBranchProtection(ctx context.Context, owner,
}

// TODO: remove custom Accept header when this API fully launches
req.Header.Set("Accept", mediaTypeProtectedBranchesPreview)
req.Header.Set("Accept", mediaTypeRequiredApprovingReviewsPreview)

return s.client.Do(ctx, req, nil)
}
Expand Down Expand Up @@ -821,7 +830,7 @@ func (s *RepositoriesService) GetPullRequestReviewEnforcement(ctx context.Contex
}

// TODO: remove custom Accept header when this API fully launches
req.Header.Set("Accept", mediaTypeProtectedBranchesPreview)
req.Header.Set("Accept", mediaTypeRequiredApprovingReviewsPreview)

r := new(PullRequestReviewsEnforcement)
resp, err := s.client.Do(ctx, req, r)
Expand All @@ -844,7 +853,7 @@ func (s *RepositoriesService) UpdatePullRequestReviewEnforcement(ctx context.Con
}

// TODO: remove custom Accept header when this API fully launches
req.Header.Set("Accept", mediaTypeProtectedBranchesPreview)
req.Header.Set("Accept", mediaTypeRequiredApprovingReviewsPreview)

r := new(PullRequestReviewsEnforcement)
resp, err := s.client.Do(ctx, req, r)
Expand Down Expand Up @@ -872,7 +881,7 @@ func (s *RepositoriesService) DisableDismissalRestrictions(ctx context.Context,
}

// TODO: remove custom Accept header when this API fully launches
req.Header.Set("Accept", mediaTypeProtectedBranchesPreview)
req.Header.Set("Accept", mediaTypeRequiredApprovingReviewsPreview)

r := new(PullRequestReviewsEnforcement)
resp, err := s.client.Do(ctx, req, r)
Expand All @@ -894,7 +903,7 @@ func (s *RepositoriesService) RemovePullRequestReviewEnforcement(ctx context.Con
}

// TODO: remove custom Accept header when this API fully launches
req.Header.Set("Accept", mediaTypeProtectedBranchesPreview)
req.Header.Set("Accept", mediaTypeRequiredApprovingReviewsPreview)

return s.client.Do(ctx, req, nil)
}
Expand All @@ -910,7 +919,7 @@ func (s *RepositoriesService) GetAdminEnforcement(ctx context.Context, owner, re
}

// TODO: remove custom Accept header when this API fully launches
req.Header.Set("Accept", mediaTypeProtectedBranchesPreview)
req.Header.Set("Accept", mediaTypeRequiredApprovingReviewsPreview)

r := new(AdminEnforcement)
resp, err := s.client.Do(ctx, req, r)
Expand All @@ -933,7 +942,7 @@ func (s *RepositoriesService) AddAdminEnforcement(ctx context.Context, owner, re
}

// TODO: remove custom Accept header when this API fully launches
req.Header.Set("Accept", mediaTypeProtectedBranchesPreview)
req.Header.Set("Accept", mediaTypeRequiredApprovingReviewsPreview)

r := new(AdminEnforcement)
resp, err := s.client.Do(ctx, req, r)
Expand All @@ -955,7 +964,7 @@ func (s *RepositoriesService) RemoveAdminEnforcement(ctx context.Context, owner,
}

// TODO: remove custom Accept header when this API fully launches
req.Header.Set("Accept", mediaTypeProtectedBranchesPreview)
req.Header.Set("Accept", mediaTypeRequiredApprovingReviewsPreview)

return s.client.Do(ctx, req, nil)
}
Expand Down
Loading