Enterprise actions permissions endpoints #2518
Conversation
| // ListEnabledOrgsInEnterprise lists the selected organizations that are enabled for GitHub Actions in an enterprise. | ||
| // | ||
| // GitHub API docs: https://docs.github.com/en/rest/actions/permissions#list-selected-organizations-enabled-for-github-actions-in-an-enterprise | ||
| func (s *ActionsService) ListEnabledOrgsInEnterprise(ctx context.Context, owner string, opts *ListOptions) (*ActionsEnabledOnEnterpriseOrgs, *Response, error) { |
There was a problem hiding this comment.
Shouldn't this be EnterpriseService here instead of ActionsService?
There was a problem hiding this comment.
My intention was for the new ActionsService.ListEnabledOrgsInEnterprise to mirror the existing ActionsService.ListEnabledReposInOrg, etc. since they implement the similar /enterprises/{enterprise}/actions/permissions/organizations and /orgs/{org}/actions/permissions/repositories endpoints respectively.
It would make more sense to have ListEnabledOrgsInEnterprise be an EnterpriseService method but should ListEnabledReposInOrg and friends be moved to OrganizationsService then to match?
There was a problem hiding this comment.
We used to determine which service an endpoint belonged to using the organization imposed by the official GitHub v3 API documentation.
Now I'm not sure that the decision is as clear-cut with the documentation site re-organization.
I'm wondering if the /enterprises/... endpoints should be in the EnterpriseService, the /orgs/... in the OrganizationsService, etc... but then am hesitant to do a complete shuffle of methods, as that is extremely disruptive to existing users.
Does anyone have thoughts or ideas on how to address this with the least amount of disruption?
There was a problem hiding this comment.
I'm wondering if the /enterprises/... endpoints should be in the EnterpriseService, the /orgs/... in the OrganizationsService, etc... but then am hesitant to do a complete shuffle of methods, as that is extremely disruptive to existing users.
In this situation i think its a bit difficult, but if documentation is added, existing users should be able to easily migrate to the new methods. If the code is oriented on the GitHub API itself, it should make it easier for new users to archieve their goal with this library, because they know the structure from playing around with the GitHub API in curl.
Therefore I think all Services should be named by their API Endpoint whenever possible.
There was a problem hiding this comment.
OK, I think my prior recommendations were incorrect. Following the documentation links, the official documentation places these endpoints under the "Actions" services, so let's please keep them there.
It looks like we now have a situation of duplicated code in two different files, so let's please resolve that problem, @algorythmic, and then we can continue with this PR.
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
Co-authored-by: Glenn Lewis <6598971+gmlewis@users.noreply.github.com>
Codecov Report
@@ Coverage Diff @@
## master #2518 +/- ##
==========================================
+ Coverage 97.97% 97.99% +0.01%
==========================================
Files 123 125 +2
Lines 10813 10919 +106
==========================================
+ Hits 10594 10700 +106
Misses 150 150
Partials 69 69
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
Is this PR still being worked on? I'd be down to help finish and close it out |
|
My team could also benefit from these changes being merged. @RickleAndMortimer, since @algorythmic is unresponsive, will you be opening a new PR to continue this work? |
|
For sure, I will set up a new PR for this |
|
Closing PR as abandoned. |
Add
/enterprises/versions of the/orgs/endpoints which have already implemented from https://docs.github.com/en/rest/actions/permissionsSince
orgs_actions_permissions.goandorgs_actions_allowed.goalready existed, just createdenterprise_*versions of those.The existing
/orgs/{org}/actions/permissions/repositoriesimplementation was inactions_runners.go, which doesn't seem to fit well. Rather than adding the enterprise version there as well, moved it out toorgs_actions_permissions.go, hope that's OK.