feat: Add fork PR workflows permission API support #3737
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3737 +/- ##
==========================================
+ Coverage 91.11% 91.14% +0.03%
==========================================
Files 187 187
Lines 16702 16764 +62
==========================================
+ Hits 15218 15280 +62
Misses 1296 1296
Partials 188 188 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Fixes: #3660. |
There was a problem hiding this comment.
Great job, @zyfy29 - thank you!
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
cc: @stevehipwell - @alexandear
github/actions_workflows.go
Outdated
| } | ||
|
|
||
| // WorkflowApprovalPolicy represents the approval policy for workflows in a repository. | ||
| type WorkflowApprovalPolicy struct { |
There was a problem hiding this comment.
Seems like we can think of a better name for the struct.
This struct is returned from the following functions:
- GetForkPRContributorApprovalPermissionsInEnterprise
- GetForkPRContributorApprovalPermissionsInOrganization
- GetForkPRContributorApprovalPermissions
And is used as a parameter in:
- UpdateForkPRContributorApprovalPermissionsInEnterprise
- UpdateForkPRContributorApprovalPermissionsInOrganization
- UpdateForkPRContributorApprovalPermissions
What about:
| type WorkflowApprovalPolicy struct { | |
| type ContributorApprovalPermissions struct { |
?
There was a problem hiding this comment.
I did hesitate here. ContributorApprovalPermissions is a little confusing but better fits the method name and api path. I agree with you.
There was a problem hiding this comment.
I think I may have missed this from the other PRs but I don't feel like the In<CONTEXT> suffix feels ergonomic. The context is significant to the API call (it's the first segment) so making it the least significant part of the function name just looks a bit off.
| // GitHub API docs: https://docs.github.com/enterprise-cloud@latest/rest/actions/permissions#get-fork-pr-contributor-approval-permissions-for-an-enterprise | ||
| // | ||
| //meta:operation GET /enterprises/{enterprise}/actions/permissions/fork-pr-contributor-approval | ||
| func (s *ActionsService) GetForkPRContributorApprovalPermissionsInEnterprise(ctx context.Context, enterprise string) (*ContributorApprovalPermissions, *Response, error) { |
There was a problem hiding this comment.
| func (s *ActionsService) GetForkPRContributorApprovalPermissionsInEnterprise(ctx context.Context, enterprise string) (*ContributorApprovalPermissions, *Response, error) { | |
| func (s *ActionsService) GetEnterpriseForkPRContributorApprovalPermissions(ctx context.Context, enterprise string) (*ContributorApprovalPermissions, *Response, error) { |
Wouldn't the Get<CONTEXT> prefix be more consistent with the existing API design?
There was a problem hiding this comment.
Yes, thank you, @stevehipwell! Let's do this, please, @zyfy29!
close #3660 since this is the last part of the permissions APIs announced there
Add support for fork PR workflow permission APIs, covering 6 endpoints.