Skip to content

Add enterprise runner group operations#2891

Merged
gmlewis merged 8 commits into
google:masterfrom
gabriel-samfira:add-enterprise-runner-group
Sep 6, 2023
Merged

Add enterprise runner group operations#2891
gmlewis merged 8 commits into
google:masterfrom
gabriel-samfira:add-enterprise-runner-group

Conversation

@gabriel-samfira

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

codecov Bot commented Aug 21, 2023

Copy link
Copy Markdown

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%> (ø)

@gmlewis gmlewis 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.

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.

Comment thread github/enterprise_actions_runner_groups.go Outdated
Signed-off-by: Gabriel Adrian Samfira <gsamfira@cloudbasesolutions.com>

@gmlewis gmlewis 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.

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

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

@WillAbides

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

gmlewis commented Aug 30, 2023

Copy link
Copy Markdown
Collaborator

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

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

@gmlewis

gmlewis commented Sep 6, 2023

Copy link
Copy Markdown
Collaborator

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
@ajschmidt8

Copy link
Copy Markdown

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.

4 participants