Skip to content

Add enterprise actions permissions endpoints and reorg files - #2920

Merged
gmlewis merged 22 commits into
google:masterfrom
RickleAndMortimer:enterprise-allowed-actions
Sep 29, 2023
Merged

Add enterprise actions permissions endpoints and reorg files#2920
gmlewis merged 22 commits into
google:masterfrom
RickleAndMortimer:enterprise-allowed-actions

Conversation

@RickleAndMortimer

Copy link
Copy Markdown
Contributor

PR following #2518

algorythmicand others added 8 commits September 11, 2023 01:37
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

codecovBot commented Sep 11, 2023

Copy link
Copy Markdown

Codecov Report

Merging #2920 (0d02f81) into master (fd33b81) will decrease coverage by 3.05%.
Report is 10 commits behind head on master.
The diff coverage is 100.00%.

@@ Coverage Diff @@## master #2920 +/- ##
==========================================
- Coverage 98.17% 95.12% -3.05% 
==========================================
Files 143 146 +3 Lines 12609 13633 +1024 ==========================================
+ Hits 12379 12969 +590 - Misses 156 559 +403 - Partials 74 105 +31 
FilesCoverage Δ
github/actions_permissions_enterprise.go100.00% <100.00%> (ø)
github/actions_permissions_orgs.go100.00% <100.00%> (ø)
github/actions_runners.go100.00% <ø> (ø)
github/orgs_actions_allowed.go100.00% <100.00%> (ø)
github/orgs_actions_permissions.go100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

@RickleAndMortimer

RickleAndMortimer commented Sep 12, 2023

Copy link
Copy Markdown
ContributorAuthor

I'm not sure what approach I should take to resolve the duplicated code. It seems like both enterprise and organization versions of the permissions just swap the url of the endpoint.

func (s *ActionsService) ListEnabledReposInOrg(ctx context.Context, owner string, opts *ListOptions) (*ActionsEnabledOnOrgRepos, *Response, error) {
u := fmt.Sprintf("orgs/%v/actions/permissions/repositories", owner)
u, err := addOptions(u, opts)
if err != nil {
return nil, nil, err
}
req, err := s.client.NewRequest("GET", u, nil)
if err != nil {
return nil, nil, err
}
repos := &ActionsEnabledOnOrgRepos{}
resp, err := s.client.Do(ctx, req, repos)
if err != nil {
return nil, resp, err
}
return repos, resp, nil
}
func (s *ActionsService) ListEnabledOrgsInEnterprise(ctx context.Context, owner string, opts *ListOptions) (*ActionsEnabledOnEnterpriseOrgs, *Response, error) {
u := fmt.Sprintf("enterprises/%v/actions/permissions/organizations", owner)
u, err := addOptions(u, opts)
if err != nil {
return nil, nil, err
}
req, err := s.client.NewRequest("GET", u, nil)
if err != nil {
return nil, nil, err
}
orgs := &ActionsEnabledOnEnterpriseOrgs{}
resp, err := s.client.Do(ctx, req, orgs)
if err != nil {
return nil, resp, err
}
return orgs, resp, nil
}

@RickleAndMortimer
RickleAndMortimer marked this pull request as ready for review September 12, 2023 04:07

@WillAbidesWillAbides left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just some suggestions to make it pass lint. I'll do a full review later today.

Comment threadgithub/enterprise_actions_allowed_test.go Outdated
Comment threadgithub/enterprise_actions_permissions_test.go Outdated
Comment threadgithub/enterprise_actions_permissions.go Outdated

@WillAbidesWillAbides left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks good except for a few doc urls that need to be updated to point to enterprise-cloud.

Comment threadgithub/enterprise_actions_allowed.go Outdated
Comment threadgithub/enterprise_actions_allowed.go Outdated
Comment threadgithub/enterprise_actions_permissions.go Outdated
Comment threadgithub/enterprise_actions_permissions.go Outdated
Comment threadgithub/enterprise_actions_permissions.go Outdated
Comment threadgithub/enterprise_actions_permissions.go Outdated
Comment threadgithub/enterprise_actions_permissions.go Outdated
Comment threadgithub/enterprise_actions_permissions.go Outdated
@gmlewis

Copy link
Copy Markdown
Collaborator

According to our CONTRIBUTING.md:


Other notes on code organization

Currently, everything is defined in the main github package, with API methods
broken into separate service objects. These services map directly to how
the GitHub API documentation is organized, so use that as your guide for
where to put new methods.

Code is organized in files also based pretty closely on the GitHub API
documentation, following the format {service}_{api}.go. For example, methods
defined at https://docs.github.com/en/rest/webhooks/repos live in
repos_hooks.go.


and I would like to preserve this convention to the best of our ability.
So I'm thinking that the ActionsService calls should be kept in files starting with actions_*.go.

Although I'm trying to catch up on a large backlog, I don't want to rush this one through without thinking about it, which is going to involve looking at the official docs and trying to make sense of which API calls belong to which Service, and therefore belong in which source file. If you would like to do the mapping, that would be great, otherwise I think I will ponder on this one until I figure out what seems to be the correct mapping according to this repo's conventions.

@gmlewis

gmlewis commented Sep 18, 2023

Copy link
Copy Markdown
Collaborator

Ah, I see that I had the same challenges with #2518 as well.

@gmlewis

Copy link
Copy Markdown
Collaborator

For reference, here are the affected endpoints and their official docs locations:

EndpointOfficial Docs URL
GET orgs/%v/actions/permissions/repositorieshttps://docs.github.com/en/rest/actions/permissions#list-selected-repositories-enabled-for-github-actions-in-an-organization
PUT orgs/%v/actions/permissions/repositorieshttps://docs.github.com/en/rest/actions/permissions#set-selected-repositories-enabled-for-github-actions-in-an-organization
PUT orgs/%v/actions/permissions/repositories/%vhttps://docs.github.com/en/rest/actions/permissions#enable-a-selected-repository-for-github-actions-in-an-organization
DELETE orgs/%v/actions/permissions/repositories/%vhttps://docs.github.com/en/rest/actions/permissions#disable-a-selected-repository-for-github-actions-in-an-organization
GET enterprises/%v/actions/permissions/selected-actionshttps://docs.github.com/enterprise-cloud@latest/rest/actions/permissions#get-allowed-actions-and-reusable-workflows-for-an-enterprise
PUT enterprises/%v/actions/permissions/selected-actionshttps://docs.github.com/enterprise-cloud@latest/rest/actions/permissions#set-allowed-actions-and-reusable-workflows-for-an-enterprise
GET enterprises/%v/actions/permissionshttps://docs.github.com/enterprise-cloud@latest/rest/actions/permissions#get-github-actions-permissions-for-an-enterprise
PUT enterprises/%v/actions/permissionshttps://docs.github.com/enterprise-cloud@latest/rest/actions/permissions#set-github-actions-permissions-for-an-enterprise
GET enterprises/%v/actions/permissions/organizationshttps://docs.github.com/enterprise-cloud@latest/rest/actions/permissions#list-selected-organizations-enabled-for-github-actions-in-an-enterprise
PUT enterprises/%v/actions/permissions/organizationshttps://docs.github.com/enterprise-cloud@latest/rest/actions/permissions#set-selected-organizations-enabled-for-github-actions-in-an-enterprise
PUT enterprises/%v/actions/permissions/organizations/%vhttps://docs.github.com/enterprise-cloud@latest/rest/actions/permissions#enable-a-selected-organization-for-github-actions-in-an-enterprise
DELETE enterprises/%v/actions/permissions/organizations/%vhttps://docs.github.com/enterprise-cloud@latest/rest/actions/permissions#disable-a-selected-organization-for-github-actions-in-an-enterprise

Next step - figure out which filename each endpoint should live in according to our stated goals in CONTRIBUTING.md... more later...

@gmlewis

Copy link
Copy Markdown
Collaborator

Next step - figure out which filename each endpoint should live in according to our stated goals in CONTRIBUTING.md... more later...

After taking the time to look at all the GitHub documentation linked in the table above, it is now clear to me that all of these endpoints should be available in the ActionsService and all endpoints should appear in the file actions_permissions.go.

Seeing that there are two flavors of endpoints (those for Orgs and those for Enterprises), I'm open to two options:

  • Option 1: place all endpoints in actions_permissions.go, or
  • Option 2: split the endpoints into actions_permissions_orgs.go for the first four endpoints in the table above and the remainder into actions_permissions_enterprise.go.

I'm fine with either, but I'm betting you might prefer Option 2. (Note that all new files should have copyright year 2023 according to the Google Open Source legal team when I was there.)

I apologize for the delay, but please proceed using either option above.

Thank you, @RickleAndMortimer !

@gmlewisgmlewis changed the title Enterprise actions permissions endpoints pt. 2Add enterprise actions permissions endpointsSep 25, 2023
@RickleAndMortimer

Copy link
Copy Markdown
ContributorAuthor

I'll run through option 2, I'll get to updating this PR asap. Thanks for the update!

@gmlewisgmlewis left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Instead of completely deleting the methods that are being moved from one service to another, can you instead please mark them as:

// Deprecated: please use `...` instead.

That way, this will not be a "Breaking API" PR.

Comment threadgithub/actions_permissions_enterprise.go Outdated
Comment threadgithub/actions_permissions_enterprise_test.go Outdated
Comment threadgithub/actions_permissions_orgs.go Outdated
Comment threadgithub/actions_permissions_orgs_test.go Outdated
Comment threadgithub/actions_permissions_enterprise_test.go Outdated
@RickleAndMortimer

Copy link
Copy Markdown
ContributorAuthor

I'm planning on just having the functions dealing with organizations as written before this PR and anything enterprise-related has it's own function, so nothing should be deprecated.

Comment threadgithub/actions_permissions_orgs.go Outdated
Comment threadgithub/actions_permissions_orgs.go Outdated
Comment threadgithub/actions_permissions_orgs.go Outdated
Comment threadgithub/actions_permissions_orgs.go Outdated
Comment threadgithub/actions_permissions_orgs_test.go
Comment threadgithub/actions_permissions_orgs_test.go Outdated
Comment threadgithub/actions_permissions_orgs_test.go Outdated
Comment threadgithub/actions_permissions_orgs_test.go Outdated
Comment threadgithub/actions_permissions_orgs_test.go Outdated
Comment threadgithub/orgs_actions_allowed.go
@RickleAndMortimer

Copy link
Copy Markdown
ContributorAuthor

Should I also add the deleted tests as well for the deprecated functions? seeing as they will be using the functions under the actions service I'm not sure if it is still necessary to do so

@gmlewis

gmlewis commented Sep 26, 2023

Copy link
Copy Markdown
Collaborator

Should I also add the deleted tests as well for the deprecated functions? seeing as they will be using the functions under the actions service I'm not sure if it is still necessary to do so

Let's please leave them for now so that our Codecov doesn't artificially drop.
(And to make sure we didn't break the deprecated methods!)

Also, I'd like to figure out why it is already dropped a few percent in this PR if we could nail that down...

@RickleAndMortimer

Copy link
Copy Markdown
ContributorAuthor

Could the loss of code coverage be from my PR being 10 commits behind? Would that fix the issue or is there something I'm missing

@gmlewis

Copy link
Copy Markdown
Collaborator

Could the loss of code coverage be from my PR being 10 commits behind? Would that fix the issue or is there something I'm missing

Hmmm... I doubt it but will investigate and get back to you...

@gmlewis

Copy link
Copy Markdown
Collaborator

Every now and then, codecov just has weird reports, and I think this is the case.
We can safely ignore it. Here's a screenshot:
pr-2920-2023-09-28_12-08-04

@RickleAndMortimer

Copy link
Copy Markdown
ContributorAuthor

Disregarding the codecov issue, Is there anything else I should include with this PR?

Comment threadgithub/orgs_actions_allowed.go Outdated
Comment threadgithub/orgs_actions_permissions.go Outdated
Comment threadgithub/orgs_actions_permissions.go Outdated
Comment threadgithub/orgs_actions_allowed.go Outdated
@gmlewis

Copy link
Copy Markdown
Collaborator

Disregarding the codecov issue, Is there anything else I should include with this PR?

Thank you for your patience and persistence with this PR, @RickleAndMortimer !
LGTM.

As far as I can tell, this is NOT a breaking-API change, which is nice.

Merging.

@gmlewisgmlewis changed the title Add enterprise actions permissions endpointsAdd enterprise actions permissions endpoints and reorg filesSep 29, 2023
@gmlewis
gmlewis merged commit e6f58e6 into google:masterSep 29, 2023
@RickleAndMortimer

Copy link
Copy Markdown
ContributorAuthor

I could say the same to you as well, thank you for your guidance!

jlaportebot added a commit to jlaportebot/go-github that referenced this pull request Jun 28, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@RickleAndMortimer@gmlewis@WillAbides@algorythmic