feat: Add support for GitHub-hosted runner API endpoints #3487

atilsensalduz · 2025-02-20T20:13:06Z

This pull request introduces support for the GitHub-hosted runner API endpoints in the go-github library, addressing Issue #3461.

Changes Include:

Implementation of API endpoints for managing GitHub-hosted runners.
Support for the following operations:
Create a new hosted runner.
Update existing hosted runner details.
Delete a hosted runner.
Retrieve details of hosted runners.
Corresponding unit tests for each operation.

Additional Information:
API Reference: GitHub REST API: Hosted Runners.

Please let me know if any additional changes or improvements are needed!

Fixes: #3461.

Implement new methods for managing GitHub-hosted runners, including functionalities for creating, updating, deleting, and retrieving details about hosted runners. References: - GitHub REST API: https://docs.github.com/en/rest/actions/hosted-runners Signed-off-by: atilsensalduz <[email protected]>

google-cla · 2025-02-20T20:13:11Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

Implement new methods for managing GitHub-hosted runners, including functionalities for creating, updating, deleting, and retrieving details about hosted runners. References: - GitHub REST API: https://docs.github.com/en/rest/actions/hosted-runners Signed-off-by: atilsensalduz <[email protected]>

codecov · 2025-02-20T21:17:13Z

Codecov Report

Attention: Patch coverage is 98.96194% with 3 lines in your changes missing coverage. Please review.

Project coverage is 91.18%. Comparing base (868f897) to head (65910b7).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
github/actions_hosted_runners.go	98.08%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3487      +/-   ##
==========================================
+ Coverage   91.02%   91.18%   +0.15%     
==========================================
  Files         179      181       +2     
  Lines       15561    15863     +302     
==========================================
+ Hits        14165    14464     +299     
- Misses       1223     1225       +2     
- Partials      173      174       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gmlewis

Thank you, @atilsensalduz !
This is looking great!
There are a few minor tweaks, please, then we should be ready for a second LGTM+Approval from any other contributor to this repo before merging.

github/actions_hosted_runners.go

github/actions_hosted_runners_test.go

Implement new methods for managing GitHub-hosted runners, including functionalities for creating, updating, deleting, and retrieving details about hosted runners. References: - GitHub REST API: https://docs.github.com/en/rest/actions/hosted-runners Signed-off-by: atilsensalduz <[email protected]>

gmlewis

Thank you, @atilsensalduz!
LGTM.

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

@stevehipwell - might you have time for a code review? Thank you!

stevehipwell

This looks great @atilsensalduz. My only question is about the distinction between the CreateHostedRunnerRequest and UpdateHostedRunnerRequest. AFAIK the idiom in the codebase is to use a single struct where possible so this differs from that pattern. Personally I like this pattern but I'm interested if @gmlewis has some overarching guidance here?

In this specific case the difference between the two models should be removed when the API catches up with the UI behaviour. So maybe a shared HostedRunnerRequest struct with an error thrown if size is set for the update until the API supports it? Or maybe a comment in code to switch to aliases when the API matches. I guess the approach here probably comes back to the question above.

gmlewis · 2025-02-25T11:42:38Z

This looks great @atilsensalduz. My only question is about the distinction between the CreateHostedRunnerRequest and UpdateHostedRunnerRequest. AFAIK the idiom in the codebase is to use a single struct where possible so this differs from that pattern. Personally I like this pattern but I'm interested if @gmlewis has some overarching guidance here?

Good catch, @stevehipwell.

We attempt to minimize the creation of extra structs and reuse similar structs for similar use cases with comments where necessary to explain usage.

…add validation Unified the CreateHostedRunnerRequest and UpdateHostedRunnerRequest into a single HostedRunnerRequest struct. Added validation functions for HostedRunnerRequest. Signed-off-by: atilsensalduz <[email protected]>

atilsensalduz · 2025-02-25T13:45:47Z

This looks great @atilsensalduz. My only question is about the distinction between the CreateHostedRunnerRequest and UpdateHostedRunnerRequest. AFAIK the idiom in the codebase is to use a single struct where possible so this differs from that pattern. Personally I like this pattern but I'm interested if @gmlewis has some overarching guidance here?

In this specific case the difference between the two models should be removed when the API catches up with the UI behaviour. So maybe a shared HostedRunnerRequest struct with an error thrown if size is set for the update until the API supports it? Or maybe a comment in code to switch to aliases when the API matches. I guess the approach here probably comes back to the question above.

@stevehipwell Thank you so much for the review and the excellent suggestion. I’ve implemented it along with the additional validations. I really appreciate your valuable input!

…add validation Unified the CreateHostedRunnerRequest and UpdateHostedRunnerRequest into a single HostedRunnerRequest struct. Added validation functions for HostedRunnerRequest. Signed-off-by: atilsensalduz <[email protected]>

gmlewis · 2025-02-25T19:18:55Z

@atilsensalduz - please run step 4 of CONTRIBUTING.md and make sure the linter and all tests pass locally, then push the changes to this PR. Thank you!

atilsensalduz · 2025-02-25T19:24:45Z

@atilsensalduz - please run step 4 of CONTRIBUTING.md and make sure the linter and all tests pass locally, then push the changes to this PR. Thank you!

Hey @gmlewis , yes, all tests passed locally. The workflow is showing a failure in the TestRepositoriesService_IsCollaborator_True test, which I didn't modify. I believe this might be a flaky test. Could you please rerun the workflow? I thought this was already a known issue, but if needed, I’d be happy to investigate and fix this flaky test

gmlewis · 2025-02-25T20:01:01Z

Hey @gmlewis , yes, all tests passed locally. The workflow is showing a failure in the TestRepositoriesService_IsCollaborator_True test, which I didn't modify. I believe this might be a flaky test. Could you please rerun the workflow? I thought this was already a known issue, but if needed, I’d be happy to investigate and fix this flaky test

Ah, very interesting! We've had some issues with GitHub Actions workflow runs lately, so if this is a flaky test it has probably been masked by these other issues... but more likely it was a GHA glitch.
It looks like the rerun worked fine. Sorry for the noise.

gmlewis

Thank you, @atilsensalduz!
After a few tweaks, please, we should be ready for a second LGTM+Approval from any other contributor to this repo.

gmlewis · 2025-02-25T20:03:00Z

github/actions_hosted_runners.go

+func (s *ActionsService) CreateHostedRunner(ctx context.Context, org string, request *HostedRunnerRequest) (*HostedRunner, *Response, error) {
+	err := validateCreateHostedRunnerRequest(request)
+	if err != nil {
+		return nil, nil, errors.New("validation failed: " + err.Error())


Suggested change

return nil, nil, errors.New("validation failed: " + err.Error())

return nil, nil, errors.New("validation failed: %w", err)

gmlewis · 2025-02-25T20:04:18Z

github/actions_hosted_runners.go

+func (s *ActionsService) UpdateHostedRunner(ctx context.Context, org string, runnerID int64, updateReq HostedRunnerRequest) (*HostedRunner, *Response, error) {
+	err := validateUpdateHostedRunnerRequest(&updateReq)
+	if err != nil {
+		return nil, nil, errors.New("validation failed: " + err.Error())


Suggested change

return nil, nil, errors.New("validation failed: " + err.Error())

return nil, nil, errors.New("validation failed: %w", err)

gmlewis · 2025-02-25T20:05:32Z

github/enterprise_actions_hosted_runners.go

+	err := validateCreateHostedRunnerRequest(request)
+	if err != nil {


Suggested change

err := validateCreateHostedRunnerRequest(request)

if err != nil {

if err := validateCreateHostedRunnerRequest(request); err != nil {

gmlewis · 2025-02-25T20:05:51Z

github/enterprise_actions_hosted_runners.go

+func (s *EnterpriseService) CreateHostedRunner(ctx context.Context, enterprise string, request *HostedRunnerRequest) (*HostedRunner, *Response, error) {
+	err := validateCreateHostedRunnerRequest(request)
+	if err != nil {
+		return nil, nil, errors.New("validation failed: " + err.Error())


Suggested change

return nil, nil, errors.New("validation failed: " + err.Error())

return nil, nil, errors.New("validation failed: %w", err)

gmlewis · 2025-02-25T20:06:17Z

github/enterprise_actions_hosted_runners.go

+	err := validateUpdateHostedRunnerRequest(&updateReq)
+	if err != nil {


Suggested change

err := validateUpdateHostedRunnerRequest(&updateReq)

if err != nil {

if err := validateUpdateHostedRunnerRequest(&updateReq); err != nil {

gmlewis · 2025-02-25T20:06:31Z

github/enterprise_actions_hosted_runners.go

+func (s *EnterpriseService) UpdateHostedRunner(ctx context.Context, enterprise string, runnerID int64, updateReq HostedRunnerRequest) (*HostedRunner, *Response, error) {
+	err := validateUpdateHostedRunnerRequest(&updateReq)
+	if err != nil {
+		return nil, nil, errors.New("validation failed: " + err.Error())


Suggested change

return nil, nil, errors.New("validation failed: " + err.Error())

return nil, nil, errors.New("validation failed: %w", err)

gmlewis changed the title ~~feat(actions): add support for GitHub-hosted runner API endpoints~~ feat: Add support for GitHub-hosted runner API endpoints Feb 20, 2025

gmlewis requested changes Feb 20, 2025

View reviewed changes

gmlewis added the NeedsReview PR is awaiting a review before merging. label Feb 20, 2025

atilsensalduz added 5 commits February 21, 2025 14:21

gmlewis approved these changes Feb 21, 2025

View reviewed changes

stevehipwell reviewed Feb 25, 2025

View reviewed changes

atilsensalduz added 2 commits February 25, 2025 17:03

gmlewis requested changes Feb 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add support for GitHub-hosted runner API endpoints #3487

feat: Add support for GitHub-hosted runner API endpoints #3487

atilsensalduz commented Feb 20, 2025 •

edited by gmlewis

Loading

google-cla bot commented Feb 20, 2025

codecov bot commented Feb 20, 2025 •

edited

Loading

gmlewis left a comment

gmlewis left a comment

stevehipwell left a comment

gmlewis commented Feb 25, 2025

atilsensalduz commented Feb 25, 2025

gmlewis commented Feb 25, 2025

atilsensalduz commented Feb 25, 2025

gmlewis commented Feb 25, 2025

gmlewis left a comment

gmlewis Feb 25, 2025

gmlewis Feb 25, 2025

gmlewis Feb 25, 2025

gmlewis Feb 25, 2025

gmlewis Feb 25, 2025

gmlewis Feb 25, 2025

	return nil, nil, errors.New("validation failed: " + err.Error())
	return nil, nil, errors.New("validation failed: %w", err)

		err := validateCreateHostedRunnerRequest(request)
		if err != nil {

	err := validateCreateHostedRunnerRequest(request)
	if err != nil {
	if err := validateCreateHostedRunnerRequest(request); err != nil {

		err := validateUpdateHostedRunnerRequest(&updateReq)
		if err != nil {

	err := validateUpdateHostedRunnerRequest(&updateReq)
	if err != nil {
	if err := validateUpdateHostedRunnerRequest(&updateReq); err != nil {

feat: Add support for GitHub-hosted runner API endpoints #3487

Are you sure you want to change the base?

feat: Add support for GitHub-hosted runner API endpoints #3487

Conversation

atilsensalduz commented Feb 20, 2025 • edited by gmlewis Loading

google-cla bot commented Feb 20, 2025

codecov bot commented Feb 20, 2025 • edited Loading

Codecov Report

gmlewis left a comment

Choose a reason for hiding this comment

gmlewis left a comment

Choose a reason for hiding this comment

stevehipwell left a comment

Choose a reason for hiding this comment

gmlewis commented Feb 25, 2025

atilsensalduz commented Feb 25, 2025

gmlewis commented Feb 25, 2025

atilsensalduz commented Feb 25, 2025

gmlewis commented Feb 25, 2025

gmlewis left a comment

Choose a reason for hiding this comment

gmlewis Feb 25, 2025

Choose a reason for hiding this comment

gmlewis Feb 25, 2025

Choose a reason for hiding this comment

gmlewis Feb 25, 2025

Choose a reason for hiding this comment

gmlewis Feb 25, 2025

Choose a reason for hiding this comment

gmlewis Feb 25, 2025

Choose a reason for hiding this comment

gmlewis Feb 25, 2025

Choose a reason for hiding this comment

atilsensalduz commented Feb 20, 2025 •

edited by gmlewis

Loading

codecov bot commented Feb 20, 2025 •

edited

Loading