-
Notifications
You must be signed in to change notification settings - Fork 927
feat: add enterprise (inherited) runner group org setting resource #3080
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
base: main
Are you sure you want to change the base?
Conversation
This resource allows managing organization-level settings for enterprise Actions runner groups that are inherited by an organization. It configures various org-specific settings such as allowed repositories, visibility and workflow restrictions. The existing org-level resource assumes that an organization runner group is to be created & fully managed by Terraform so it does not work for an enterprise-scope group that is shared with one or more organizations within a GitHub Enterprise environment. Closes integrations#2768
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
|
Thanks for opening a PR!
I was proven wrong, separate resources are better |
| Create: resourceGithubEnterpriseActionsRunnerGroupOrgSettingsCreate, | ||
| Read: resourceGithubEnterpriseActionsRunnerGroupOrgSettingsRead, | ||
| Update: resourceGithubEnterpriseActionsRunnerGroupOrgSettingsUpdate, | ||
| Delete: resourceGithubEnterpriseActionsRunnerGroupOrgSettingsDelete, | ||
| Importer: &schema.ResourceImporter{ | ||
| State: resourceGithubEnterpriseActionsRunnerGroupOrgSettingsImport, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: Please use Context-aware provider functions
| Importer: &schema.ResourceImporter{ | ||
| State: resourceGithubEnterpriseActionsRunnerGroupOrgSettingsImport, | ||
| }, | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a top-level Description field
| "github_enterprise_actions_runner_group": resourceGithubActionsEnterpriseRunnerGroup(), | ||
| "github_enterprise_actions_workflow_permissions": resourceGithubEnterpriseActionsWorkflowPermissions(), | ||
| "github_actions_organization_workflow_permissions": resourceGithubActionsOrganizationWorkflowPermissions(), | ||
| "github_enterprise_actions_runner_group_org_settings": resourceGithubEnterpriseActionsRunnerGroupOrgSettings(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of using abbreviations in the resource names, what about something like resource_github_organization_inherited_runner_group_settings?
| "enterprise_runner_group_name": { | ||
| Type: schema.TypeString, | ||
| Required: true, | ||
| ForceNew: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if ForceNew here is correct, since the name of a runner group can change without changing the ID. Maybe this should instead have a CustomizeDiff function which checks if name was changed AND runner_group_id changed => ForceNew
|
|
||
| for _, group := range groups.RunnerGroups { | ||
| // Only match runner groups that are inherited from the enterprise | ||
| if group.GetName() == name && group.GetInherited() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: A minor optimization would be to swat the conditions here. Checking GetInherited() first should be "cheaper" than doing a string comparison
| } | ||
|
|
||
| func resourceGithubEnterpriseActionsRunnerGroupOrgSettingsImport(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) { | ||
| parts := strings.Split(d.Id(), ":") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| parts := strings.Split(d.Id(), ":") | |
| org, identifier, err := parseID2(d.Id()) |
| func resourceGithubEnterpriseActionsRunnerGroupOrgSettingsImport(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) { | ||
| parts := strings.Split(d.Id(), ":") | ||
| if len(parts) != 2 { | ||
| return nil, fmt.Errorf("invalid import ID format, expected 'organization:enterprise_runner_group_name' or 'organization:runner_group_id'") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user needs to understand that they can't use the runner group ID from the enterprise, but the Org. They are different 😬
| return nil, fmt.Errorf("invalid import ID format, expected 'organization:enterprise_runner_group_name' or 'organization:runner_group_id'") | |
| return nil, fmt.Errorf("invalid import ID format, expected 'organization:enterprise_runner_group_name' or 'organization:organization_runner_group_id'") |
| if isEnterprise != "true" { | ||
| t.Skip("Skipping because `ENTERPRISE_ACCOUNT` is not set or set to false") | ||
| } | ||
|
|
||
| if testEnterprise == "" { | ||
| t.Skip("Skipping because `ENTERPRISE_SLUG` is not set") | ||
| } | ||
|
|
||
| if testOrganization == "" { | ||
| t.Skip("Skipping because `GITHUB_ORGANIZATION` is not set") | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use the new skipUnless* functions, you don't need to worry about checking these fields
| if isEnterprise != "true" { | |
| t.Skip("Skipping because `ENTERPRISE_ACCOUNT` is not set or set to false") | |
| } | |
| if testEnterprise == "" { | |
| t.Skip("Skipping because `ENTERPRISE_SLUG` is not set") | |
| } | |
| if testOrganization == "" { | |
| t.Skip("Skipping because `GITHUB_ORGANIZATION` is not set") | |
| } |
|
|
||
| # Create a test repository | ||
| resource "github_repository" "test" { | ||
| name = "tf-acc-test-repo-%s" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use testResourcePrefix to generate the repo name
| t.Run("with an enterprise account", func(t *testing.T) { | ||
| testCase(t, enterprise) | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This structure is no longer supported
| t.Run("with an enterprise account", func(t *testing.T) { | |
| testCase(t, enterprise) | |
| }) |
Add new resource
github_enterprise_actions_runner_group_org_settings.This resource allows managing organization-level settings for enterprise
Actions runner groups that are inherited by an organization. It configures
various org-specific settings such as allowed repositories, visibility and
workflow restrictions. The existing org-level resource assumes that an
organization runner group is to be created & fully managed by Terraform so
it does not work for an enterprise-scope group that is shared with one or
more organizations within a GitHub Enterprise environment.
Resolves #2768 (#2768)
Before the change?
After the change?
github_enterprise_actions_runner_group_org_settingscan be createdto manage an enterprise-scoped, inherited runner group for a GitHub organization. Note that a pre-requisite resource
github_enterprise_actions_runner_groupis required and is depended on existing before org settings can be created & managed.Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!