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 runner group operations #2891

Merged
merged 8 commits into from Sep 6, 2023

Conversation

gabriel-samfira
Copy link
Contributor

This change adds runner group operations on enterprises. The operations are essentially a mirror of the org runner group code, adapted for enterprises.

This change adds runner group operations on enterprises.

Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
@codecov
Copy link

codecov bot commented Aug 21, 2023

Codecov Report

Merging #2891 (428e724) into master (b700431) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2891      +/-   ##
==========================================
+ Coverage   98.12%   98.14%   +0.02%     
==========================================
  Files         142      143       +1     
  Lines       12453    12593     +140     
==========================================
+ Hits        12219    12359     +140     
  Misses        159      159              
  Partials       75       75              
Files Changed Coverage Δ
github/enterprise_actions_runner_groups.go 100.00% <100.00%> (ø)

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.

Thank you, @gabriel-samfira for the very clean and well-written PR!
❤️
Just one tiny nit, please, then we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.

github/enterprise_actions_runner_groups.go Outdated Show resolved Hide resolved
Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
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.

Thank you, @gabriel-samfira !
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

@gmlewis gmlewis added the NeedsReview PR is awaiting a review before merging. label Aug 21, 2023
Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>
@gabriel-samfira
Copy link
Contributor Author

I had forgotten about the omitempty tag. Added it now.

@WillAbides
Copy link
Contributor

For consistency with ActionsService, should the "*RunnerGroup" methods be renamed to "*EnterpriseRunnerGroup"?

This PR also takes the opposite approach with the "*GroupRunners" methods. ActionsService doesn't include "Organization" in those method names, but EnterpriseService includes "Enterprise". I think the ActionsService names make more sense because they are acting on a "Runner", not an "OrganizationRunner" or "EnterpriseRunner".

@gmlewis
Copy link
Collaborator

gmlewis commented Aug 30, 2023

For consistency with ActionsService, should the "*RunnerGroup" methods be renamed to "*EnterpriseRunnerGroup"?

This PR also takes the opposite approach with the "*GroupRunners" methods. ActionsService doesn't include "Organization" in those method names, but EnterpriseService includes "Enterprise". I think the ActionsService names make more sense because they are acting on a "Runner", not an "OrganizationRunner" or "EnterpriseRunner".

Excellent points, @WillAbides - I think you are right.
@gabriel-samfira - would you mind making these naming changes, please?

@gabriel-samfira
Copy link
Contributor Author

Ohh wow. For some reason I did not get a notification for your comments. I will make these changes ASAP. Thanks for the review!

@gabriel-samfira
Copy link
Contributor Author

Hopefully I got all the names right. Let me know if I missed (or misunderstood) anything.

@gmlewis
Copy link
Collaborator

gmlewis commented Sep 6, 2023

Thank you, @WillAbides !
Merging.

@gmlewis gmlewis removed the NeedsReview PR is awaiting a review before merging. label Sep 6, 2023
@gmlewis gmlewis merged commit 4030e93 into google:master Sep 6, 2023
9 checks passed
@ajschmidt8
Copy link

Appreciate the work here! My team will benefit from these changes as well.

Thanks, everyone.

@gabriel-samfira gabriel-samfira deleted the add-enterprise-runner-group branch September 7, 2023 19:21
gmlewis pushed a commit to gmlewis/go-github that referenced this pull request Sep 19, 2023
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