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.

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 enterprise actions permissions endpoints and reorg files #2920

Merged
merged 22 commits into from Sep 29, 2023

Conversation

RickleAndMortimer
Copy link
Contributor

PR following #2518

algorythmic and 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
Copy link

codecov bot commented Sep 11, 2023

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     
Files Coverage Δ
github/actions_permissions_enterprise.go 100.00% <100.00%> (ø)
github/actions_permissions_orgs.go 100.00% <100.00%> (ø)
github/actions_runners.go 100.00% <ø> (ø)
github/orgs_actions_allowed.go 100.00% <100.00%> (ø)
github/orgs_actions_permissions.go 100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

@RickleAndMortimer
Copy link
Contributor Author

RickleAndMortimer commented Sep 12, 2023

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
Copy link
Contributor

@WillAbides WillAbides left a comment

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.

github/enterprise_actions_allowed_test.go Outdated Show resolved Hide resolved
github/enterprise_actions_permissions_test.go Outdated Show resolved Hide resolved
github/enterprise_actions_permissions.go Outdated Show resolved Hide resolved
Copy link
Contributor

@WillAbides WillAbides left a comment

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.

github/enterprise_actions_allowed.go Outdated Show resolved Hide resolved
github/enterprise_actions_allowed.go Outdated Show resolved Hide resolved
github/enterprise_actions_permissions.go Outdated Show resolved Hide resolved
github/enterprise_actions_permissions.go Outdated Show resolved Hide resolved
github/enterprise_actions_permissions.go Outdated Show resolved Hide resolved
github/enterprise_actions_permissions.go Outdated Show resolved Hide resolved
github/enterprise_actions_permissions.go Outdated Show resolved Hide resolved
github/enterprise_actions_permissions.go Outdated Show resolved Hide resolved
@gmlewis
Copy link
Collaborator

gmlewis commented Sep 18, 2023

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
Copy link
Collaborator

gmlewis commented Sep 18, 2023

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

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 20, 2023

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

Endpoint Official Docs URL
GET orgs/%v/actions/permissions/repositories https://docs.github.com/en/rest/actions/permissions#list-selected-repositories-enabled-for-github-actions-in-an-organization
PUT orgs/%v/actions/permissions/repositories https://docs.github.com/en/rest/actions/permissions#set-selected-repositories-enabled-for-github-actions-in-an-organization
PUT orgs/%v/actions/permissions/repositories/%v https://docs.github.com/en/rest/actions/permissions#enable-a-selected-repository-for-github-actions-in-an-organization
DELETE orgs/%v/actions/permissions/repositories/%v https://docs.github.com/en/rest/actions/permissions#disable-a-selected-repository-for-github-actions-in-an-organization
GET enterprises/%v/actions/permissions/selected-actions https://docs.github.com/enterprise-cloud@latest/rest/actions/permissions#get-allowed-actions-and-reusable-workflows-for-an-enterprise
PUT enterprises/%v/actions/permissions/selected-actions https://docs.github.com/enterprise-cloud@latest/rest/actions/permissions#set-allowed-actions-and-reusable-workflows-for-an-enterprise
GET enterprises/%v/actions/permissions https://docs.github.com/enterprise-cloud@latest/rest/actions/permissions#get-github-actions-permissions-for-an-enterprise
PUT enterprises/%v/actions/permissions https://docs.github.com/enterprise-cloud@latest/rest/actions/permissions#set-github-actions-permissions-for-an-enterprise
GET enterprises/%v/actions/permissions/organizations https://docs.github.com/enterprise-cloud@latest/rest/actions/permissions#list-selected-organizations-enabled-for-github-actions-in-an-enterprise
PUT enterprises/%v/actions/permissions/organizations https://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/%v https://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/%v https://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
Collaborator

gmlewis commented Sep 25, 2023

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 !

@gmlewis gmlewis changed the title Enterprise actions permissions endpoints pt. 2 Add enterprise actions permissions endpoints Sep 25, 2023
@RickleAndMortimer
Copy link
Contributor Author

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

Copy link
Collaborator

@gmlewis gmlewis left a comment

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.

github/actions_permissions_enterprise.go Outdated Show resolved Hide resolved
github/actions_permissions_enterprise_test.go Outdated Show resolved Hide resolved
github/actions_permissions_orgs.go Outdated Show resolved Hide resolved
github/actions_permissions_orgs_test.go Outdated Show resolved Hide resolved
github/actions_permissions_enterprise_test.go Outdated Show resolved Hide resolved
@RickleAndMortimer
Copy link
Contributor Author

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.

github/actions_permissions_orgs.go Outdated Show resolved Hide resolved
github/actions_permissions_orgs.go Outdated Show resolved Hide resolved
github/actions_permissions_orgs.go Outdated Show resolved Hide resolved
github/actions_permissions_orgs.go Outdated Show resolved Hide resolved
github/actions_permissions_orgs_test.go Show resolved Hide resolved
github/actions_permissions_orgs_test.go Outdated Show resolved Hide resolved
github/actions_permissions_orgs_test.go Outdated Show resolved Hide resolved
github/actions_permissions_orgs_test.go Outdated Show resolved Hide resolved
github/actions_permissions_orgs_test.go Outdated Show resolved Hide resolved
github/orgs_actions_allowed.go Show resolved Hide resolved
@RickleAndMortimer
Copy link
Contributor Author

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
Copy link
Collaborator

gmlewis commented Sep 26, 2023

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
Contributor Author

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
Collaborator

gmlewis commented Sep 28, 2023

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
Collaborator

gmlewis commented Sep 28, 2023

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
Contributor Author

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

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 29, 2023

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.

@gmlewis gmlewis changed the title Add enterprise actions permissions endpoints Add enterprise actions permissions endpoints and reorg files Sep 29, 2023
@gmlewis gmlewis merged commit e6f58e6 into google:master Sep 29, 2023
6 of 7 checks passed
@RickleAndMortimer
Copy link
Contributor Author

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

Sign up for free to 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.

None yet

4 participants