New Resource: github_actions_runner_group #821
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Another, more general question: when I try to run any preexisting test that includes the My And I believe the token has the needed scopes: In case it helps, my env should be configured as described in CONTRIBUTING.md: (Marking as ready for review to request feedback) |
|
The |
|
@jcudit could you enable workflows for @soulshake? I recall you saying some tests can only be run by you (?). |
|
What is the best way to move this forward and get support for the Actions API into the provider? |
|
Apologies for the inactivity here, lets get this ready to ship! Please cherry-pick b5b793a in order to get the tests aligned to the project's testing suite. I've tested this branch on the latest GHES release as well as with a GHEC account. Functionality seems 👌🏾 for GHEC but we are lacking API support for GHES as of 3.1: ---[ REQUEST ]---------------------------------------
POST /api/v3/orgs/terraformtesting/actions/runner-groups HTTP/1.1
Host: jcudit-1628516972.ghe-test.com
User-Agent: go-github
Content-Length: 48
Accept: application/vnd.github.v3+json
Content-Type: application/json
Accept-Encoding: gzip
{
"name": "tf-acc-test-e4zrk",
"visibility": "all"
}
-----------------------------------------------------
2021/08/09 10:41:33 [DEBUG] Github API Response Details:
---[ RESPONSE ]--------------------------------------
HTTP/2.0 404 Not Found
Access-Control-Allow-Origin: *
Access-Control-Expose-Headers: ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Used, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type, Deprecation, Sunset
Content-Security-Policy: default-src 'none'
Content-Type: application/json; charset=utf-8
Date: Mon, 09 Aug 2021 14:41:33 GMT
Referrer-Policy: origin-when-cross-origin, strict-origin-when-cross-origin
Server: github.com
Strict-Transport-Security: max-age=31536000; includeSubdomains
X-Accepted-Oauth-Scopes: repo
X-Content-Type-Options: nosniff
X-Frame-Options: deny
X-Github-Enterprise-Version: 3.1.4
X-Github-Media-Type: github.v3; format=json
X-Github-Request-Id: 9367b480-ff75-41ba-a46a-1819e252a0a2
X-Oauth-Scopes: admin:enterprise, admin:gpg_key, admin:org, admin:org_hook, admin:pre_receive_hook, admin:public_key, admin:repo_hook, delete:packages, delete_repo, gist, notifications, repo, site_admin, user, workflow, write:discussion, write:packages
X-Runtime-Rack: 0.018059
X-Xss-Protection: 1; mode=block
{
"message": "Not Found",
"documentation_url": "https://docs.github.com/enterprise/3.1/rest"
}As for the visibility configuration, this is the most troublesome bit. Perhaps we can ship this resource with limited support and revisit setting visibility to We will also need to add docs for the new resource before this can merge. Adding these will allow us to communicate limitations around GHES use and the |
soulshake
force-pushed
the
organization_runner_group
branch
from
f89cd45
to
da349c1
Compare
|
@jcudit Thanks! I've added docs and a few more tests. Please let me know if this is what you had in mind. The tests seem to be passing: Test outputHowever, I haven't been able to actually test the provider locally; not sure what I'm doing wrong: Attempt at using built providerAfter adding required_providers block |
soulshake
commented
Aug 18, 2021
Comment on lines
171
to
179
| log.Printf("[DEBUG] Reading organization runner group repositories: %s (%s)", d.Id(), orgName) | ||
| runnerGroupRepositories, resp, err := client.Actions.ListRepositoryAccessRunnerGroup(ctx, orgName, runnerGroupID) | ||
|
|
||
| selectedRepositoryIDs := []int64{} | ||
| for _, repo := range runnerGroupRepositories.Repositories { | ||
| selectedRepositoryIDs = append(selectedRepositoryIDs, *repo.ID) | ||
| } | ||
| log.Printf("[DEBUG] Got selected_repository_ids: %v", selectedRepositoryIDs) | ||
| d.Set("selected_repository_ids", selectedRepositoryIDs) |
There was a problem hiding this comment.
I did not see a way to get the selected_repository_ids without making this additional API call.
soulshake
commented
Aug 18, 2021
|
|
||
| t.Run("creates runner groups without error", func(t *testing.T) { | ||
|
|
||
| // t.Skip("requires an enterprise cloud account") |
There was a problem hiding this comment.
Since the tests don't run when this line is present, I wasn't sure if it should be there or not.
soulshake
commented
Aug 18, 2021
Comment on lines
+224
to
+234
| t.Skip("always shows a diff for visibility 'all' => 'private'") | ||
| testCase(t, organization) |
There was a problem hiding this comment.
I wasn't sure if I should leave this test here or take it out entirely.
There was a problem hiding this comment.
Safe to leave in for this first pass. We may upgrade our testing suite and revisit in the future 🤞🏾
jcudit
reviewed
Sep 2, 2021
There was a problem hiding this comment.
This is pretty close and will likely ship with the next release. Can we get 5ec99a3 cherry-picked in to finalize documentation?
Aiming to play more with the test suite against different versions of enterprise deployments. Hoping to get limitations documented and flush out any obvious bugs.
Excellent effort 🙇🏾 !
soulshake
force-pushed
the
organization_runner_group
branch
from
3dc6c65
to
edfd706
Compare
|
@jcudit cherry-pick has been done, thank you! |
jcudit
approved these changes
Sep 12, 2021
|
@jcudit Suggestion merged 👍 Thanks! |
jcudit
approved these changes
Sep 13, 2021
|
Ah, one more round of fixes 🙃 Can we get the conflicts removed here as another PR was merged that added a new resource. |
Co-authored-by: Jeremy Udit <jcudit@github.com>
soulshake
force-pushed
the
organization_runner_group
branch
from
a96f4dd
to
320ddb3
Compare
Of course! Done and done |
jcudit
reviewed
Sep 21, 2021
kfcampbell
pushed a commit
to kfcampbell/terraform-provider-github
that referenced
this pull request
Jul 26, 2022
* WIP * fixup! add tests compatible with test suite * Add doc * Set selected_repository_ids * Add import tests * Add note to docs regarding unsupported `visibility = private` * Add check for selected_repository_ids.# when importing a 'selected' runner group * Add a few more name checks * fixup! link new resource documentation * Update github/resource_github_actions_runner_group.go Co-authored-by: Jeremy Udit <jcudit@github.com> * fixup! handle error to satisfy linter * fixup! discard unused return value Co-authored-by: Jeremy Udit <jcudit@github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
(Disclaimer: this is my first time working on a Terraform provider, and this PR is still a work in progress)
This PR is intended to add support for self-hosted runner groups in GitHub Actions as described in https://docs.github.com/en/rest/reference/actions#self-hosted-runner-groups.
My questions
visibilitywhich should acceptprivateas an option. But I can't get a runner group withvisibility = privatethrough any combination ofPOSTorPATCHcalls I've tried (details below). Should I still attempt to add support for this?allows_public_repositoriesparameter, but providing this parameter seems to have an effect. Should I add support for this?1. visibility = private?
I added tests, but the one that tests
visibility = trueis failing.Output of failing test for a runner group with `visibility = private`
After testing manually with
curlto debug this failing test, I'm confused about the expected behavior of thePOST /orgs/{org}/actions/runner-groupsmethod. The API returns a runner group with visibilityall, despite specifying"visibility": "private"in the request:It may be worth mentioning that the dropdown on the web UI only has options for "Selected" and "All" (whether updating a runner group or creating a new runner group):
(There is also a checkmark--hidden by the dropdown in the screenshot above--for "Allow public repositories" that is checkable whether "selected" or "all" is chosen from the dropdown.)
Am I missing something?
2.
allows_public_repositoriesparameter?There is no documented
allows_public_repositoriesparameter (ref), but specifying it seems to be the only way to get a value ofallows_public_repositories": truein thecreateresponse:Related: #542