feat: Add github_organization_security_configuration and github_enterprise_security_configuration resource#3284
Conversation
|
👋 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 |
…prise_security_configuration resources Adds two new resources to manage Code Security Configurations: - github_organization_security_configuration: manages code security configurations at the organization level - github_enterprise_security_configuration: manages code security configurations at the enterprise level Both resources include: - Full CRUD operations using GitHub's Code Security Configurations API - Composite IDs (org/enterprise + config ID) - 404-tolerant delete - tflog structured logging throughout - All optional fields use GetOk to avoid sending unset values - Custom import support - Shared expandCodeSecurityConfigurationCommon helper to avoid duplication - All 4 delegated fields on enterprise: code_scanning_delegated_alert_dismissal, secret_scanning_delegated_bypass, secret_scanning_delegated_bypass_options, secret_scanning_delegated_alert_dismissal - Fix flattenCodeScanningDefaultSetupOptions runner_type empty string drift Acceptance tests (5 per resource): - creates without error (with import verification) - updates without error - creates with nested options (runner, autosubmit) - creates with minimal config (with import verification) - creates with delegated bypass options Documentation added for both resources. Resolves integrations#2412 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ppers - Add setCodeSecurityConfigurationState() to util_security_configuration.go, replacing ~83 identical d.Set() lines duplicated across both Read functions - Remove expandCodeSecurityConfiguration() and expandEnterpriseCodeSecurityConfiguration() one-liner wrappers; callers now call expandCodeSecurityConfigurationCommon() directly Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove fmt.Sprintf from all tflog calls; use static messages with structured fields map for dynamic data (28 instances fixed) - Add configuration_id Computed field to both resources so the numeric config ID is stored separately in state - Update/Delete now read enterprise_slug and configuration_id from state via d.Get() instead of parsing the composite ID - Update enterprise docs with configuration_id attribute Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…n_id - Add missing organization_security_configuration documentation - Fix enterprise docs: description is Optional not Required - Add configuration_id assertions to both test files Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
8b5e69b to
20f4c17
Compare
…view feedback - Upgrade go-github imports from v83 to v84 across all feature files - Remove secret_scanning_delegated_bypass from enterprise resource (org-only API) - Fix reviewer_type enum casing to TEAM/ROLE to match GitHub API - Wire expandSecretScanningDelegatedBypass into org Create/Update - Remove hardcoded "disabled" defaults for code_security/secret_protection - Use GetOk for description field in expand (consistency with other Optional fields) - Add unit tests for all flatten utility functions (deiga requested) - Add missing ImportState steps to acceptance tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
@deiga ready for review. Thanks for your patience! I have allowed edits by maintainers. |
deiga
left a comment
There was a problem hiding this comment.
Partial review.
Please take some time and look at lately merged PRs to understand what kind of structures we are looking for.
I don't have the energy to review thousands of lines of code, when you haven't put in the work to adhere to the standards of the repo
…nventions - Add custom importer functions so Read doesn't parse from ID - Read fetches org/enterprise and configuration_id from state - Create/Update return nil instead of calling Read directly - Use diags.HasError() instead of diags != nil - Use testResourcePrefix in all test resource names - Extract import tests into separate t.Run blocks - Inline test HCL templates instead of shared tmpl variables
… Read Create and Update functions now set state directly from the API response via setCodeSecurityConfigurationState, rather than only setting configuration_id. Enterprise Update also captures the API response instead of discarding it.
…escription - Add CheckDestroy functions to all acceptance tests for both org and enterprise security configuration resources - Cast configuration.GetID() to int to match schema.TypeInt - Fix redundant "code security configuration for the code security configuration" description on the code_security field
|
Reviewed and looking at the recent PRs to add in the changes the maintainers are implementing to try and make it consistent. I hope we are much closer this time. |
Use single template string in enterprise update test, remove superfluous buildID/SetId in import functions, refactor util tests to table-driven arrays.
Adds table-driven tests for expandCodeSecurityConfigurationCommon and expandSecretScanningDelegatedBypass, covering minimal input, all string fields, nested block options, and delegated bypass with reviewers.
Drop the org name from the compound resource ID since it was never used — Read/Update/Delete all get the org from meta.(*Owner).name. This aligns with the convention used by other org-scoped resources (organization_ruleset, organization_webhook, organization_custom_role, etc.).
|
@deiga Thank you for taking the time to review this PR and for the detailed feedback across each round. All comments have been addressed. Looking forward to getting this feature out for the GitHub community! |
…pwell-review Address @stevehipwell review: code idioms, lint, docs
Thank you again @casey-robertson-paypal for your work helping with this PR. I have approved and merged the PR now. The judgement call is up to @stevehipwell and the conventions they want to use for the maturing repo. |
stevehipwell
left a comment
There was a problem hiding this comment.
Looking through this PR I don't think Optional & Computed is required on the config attributes. Either just Optional or Optional & Default would reduce the complexity of the logic and also reduce churn. I think I'm leaning towards just Optional.
Also as mentioned in my previous review, any shared state get/set logic should live with and be scoped to the resource code as it's fundamental to the resource logic.
FYI security_configuration.go needs to be renamed to util_security_configuration.go for consistency.
- Restore util_ filename: security_configuration.go -> util_security_configuration.go
(maintainer asked for util_ for consistency with the repo's other util_ files).
- Colocate state get/set logic into the resource files: each resource now has its
own resourceGithub<Org|Enterprise>SecurityConfiguration{SetState,Expand}; the
shared util_ file keeps only the pure flatten helpers. Org variants include the
org-only delegated-bypass fields; enterprise variants are common-only.
- Schema: make 'description' Optional only (drop Computed). Verified against the
GitHub API that an omitted description is returned null (not server-defaulted),
so Computed is unnecessary and Optional causes no drift. The enum config
attributes keep Optional+Computed because GitHub server-assigns values to them
when omitted, so dropping Computed there would cause perpetual diffs.
|
@sprioriello sprioriello/terraform-provider-github#5 is ready for review/merge — it addresses @stevehipwell's round-2 feedback:
On the broader Verified: |
Acceptance test results (real GHAS org)Since the acceptance suite only runs in CI under the
Toolchain: Coverage caveat — enterprise resourceThe enterprise acceptance tests ( NotesThe org create-with-delegated-bypass-reviewers path hits a GitHub REST API bug ( |
|
@stevehipwell on your data-sources question — opened #3505 to track matching data sources for both configurations as a separate follow-up, keeping this PR focused on the resources. Happy to pick up the implementation once these land. |
|
Filed the GitHub API 500 (delegated-bypass create) upstream: https://github.com/orgs/community/discussions/200071 — for tracking once GitHub fixes it and the create-time path can be restored. |
…lean Address @stevehipwell round-2 review: colocate state logic, util_ filename, description Optional
Approved and merged |
@casey-robertson-paypal I think the issue here is that the logic to handle |
…values Per @stevehipwell's review: remove Computed from the optional config attributes so they are Optional only (configuration_id and target_type stay Computed). To avoid perpetual diffs (GitHub server-assigns a value to every attribute even when omitted), setState now only writes attributes the user manages — a non-empty value in config on create/update, or in prior state on read (via d.Get). Unmanaged values are not tracked in state and can be read with the data source instead. Import consequence: with no config present, import populates the identifying fields (configuration_id, name, target_type); attributes are then declared in config. Tests adjusted accordingly (import test uses a name-only config; the delegated-bypass nested-block import step is removed, as unmanaged/nested values don't round-trip in this model). Verified: org acceptance suite 6/6 with clean post-apply plans; go build / vet / golangci-lint / gofmt all clean.
|
Makes sense @stevehipwell — conceding the point. Implemented in sprioriello/terraform-provider-github#6 (for @sprioriello to merge): To keep plans clean without One thing I'd value your steer on: with no config during import, there's no user intent to read, so I had import populate the identifying fields ( I'm going to be on vacation through July 5th but I'll check back in the following week. Thanks for the reviews and the guidance! |
Drop Computed from config attributes (Optional only), per @stevehipwell
stevehipwell
left a comment
There was a problem hiding this comment.
I think the code can be simplified further now the inputs are optional and no longer computed. I've only reviewed the enterprise resource, but the comments apply to both.
Per @stevehipwell's round-4 review: - Optional (non-computed) attributes are persisted from config by the SDK, so Create only writes the computed values (configuration_id, target_type) inline, and Update writes nothing back. - Read is the only place that reconciles state with the API: inlined, using d.GetOk as the primary filter so only managed attributes are refreshed (the removed resourceGithub*SetState helper is no longer needed). - Make the sole bool in each optional block (labeled_runners, allow_advanced) Required rather than Optional, since optional bools are problematic. Org acceptance suite 6/6; go build / vet / golangci-lint / gofmt clean; docs regenerated.
|
@sprioriello sprioriello/terraform-provider-github#7 is up for merge — it implements @stevehipwell's round-4 simplification (applied to both resources): Create writes only the computed values, Update writes nothing back, Read reconciles via |
The enterprise import test set a description; under the Optional-only model import does not round-trip unmanaged attributes, so ImportStateVerify would fail. Mirror the org import test (identifying fields only). Enterprise acc tests skip without a live enterprise, so this only surfaced in review.
Simplify CRUD now that config attributes are Optional-only (@stevehipwell round 4)
|
@stevehipwell thanks to the work of @casey-robertson-paypal your last comments have been addressed and ready for another review. Looking forward to getting merged and released. |
stevehipwell
left a comment
There was a problem hiding this comment.
This is looking good, I've added some comments and please apply them across all of the code not just the instances I commented on.
| Description: "Manages a code security configuration for a GitHub Enterprise.", | ||
| CreateContext: resourceGithubEnterpriseSecurityConfigurationCreate, | ||
| ReadContext: resourceGithubEnterpriseSecurityConfigurationRead, | ||
| UpdateContext: resourceGithubEnterpriseSecurityConfigurationUpdate, | ||
| DeleteContext: resourceGithubEnterpriseSecurityConfigurationDelete, | ||
| Importer: &schema.ResourceImporter{ | ||
| StateContext: resourceGithubEnterpriseSecurityConfigurationImport, | ||
| }, |
There was a problem hiding this comment.
| Description: "Manages a code security configuration for a GitHub Enterprise.", | |
| CreateContext: resourceGithubEnterpriseSecurityConfigurationCreate, | |
| ReadContext: resourceGithubEnterpriseSecurityConfigurationRead, | |
| UpdateContext: resourceGithubEnterpriseSecurityConfigurationUpdate, | |
| DeleteContext: resourceGithubEnterpriseSecurityConfigurationDelete, | |
| Importer: &schema.ResourceImporter{ | |
| StateContext: resourceGithubEnterpriseSecurityConfigurationImport, | |
| }, | |
| CreateContext: resourceGithubEnterpriseSecurityConfigurationCreate, | |
| ReadContext: resourceGithubEnterpriseSecurityConfigurationRead, | |
| UpdateContext: resourceGithubEnterpriseSecurityConfigurationUpdate, | |
| DeleteContext: resourceGithubEnterpriseSecurityConfigurationDelete, | |
| Importer: &schema.ResourceImporter{ | |
| StateContext: resourceGithubEnterpriseSecurityConfigurationImport, | |
| }, | |
| Description: "Resource to manage a GitHub code security configuration for an enterprise.", |
| Type: schema.TypeString, | ||
| Required: true, | ||
| ForceNew: true, | ||
| Description: "The slug of the enterprise.", |
There was a problem hiding this comment.
| Description: "The slug of the enterprise.", | |
| Description: "Enterprise slug.", |
The repetition of "The ..." in descriptions is best avoided if possible.
| {"enforcement", configuration.GetEnforcement()}, | ||
| } | ||
| for _, attr := range managed { | ||
| if _, ok := d.GetOk(attr.key); !ok { |
There was a problem hiding this comment.
Please add a comment here to the effect that this pattern does not support bool types as d.GetOk for a bool field will always return true.
| func TestAccGithubEnterpriseSecurityConfiguration(t *testing.T) { | ||
| t.Run("creates enterprise security configuration without error", func(t *testing.T) { |
There was a problem hiding this comment.
| func TestAccGithubEnterpriseSecurityConfiguration(t *testing.T) { | |
| t.Run("creates enterprise security configuration without error", func(t *testing.T) { | |
| func TestAccGithubEnterpriseSecurityConfiguration(t *testing.T) { | |
| t.Parallel() | |
| skipUnlessEnterprise(t) | |
| t.Run("creates enterprise security configuration without error", func(t *testing.T) { | |
| t.Parallel() |
The test code pattern has been updated so please make the tests parallel. Also please remove any shared skips from the PreCheck and set them at the top of the root test.
| statecheck.ExpectKnownValue("github_enterprise_security_configuration.test", | ||
| tfjsonpath.New("name"), knownvalue.StringExact(configName)), | ||
| statecheck.ExpectKnownValue("github_enterprise_security_configuration.test", | ||
| tfjsonpath.New("description"), knownvalue.StringExact("Test configuration")), | ||
| statecheck.ExpectKnownValue("github_enterprise_security_configuration.test", | ||
| tfjsonpath.New("advanced_security"), knownvalue.StringExact("enabled")), | ||
| statecheck.ExpectKnownValue("github_enterprise_security_configuration.test", | ||
| tfjsonpath.New("enforcement"), knownvalue.StringExact("enforced")), | ||
| statecheck.ExpectKnownValue("github_enterprise_security_configuration.test", | ||
| tfjsonpath.New("configuration_id"), knownvalue.NotNull()), |
There was a problem hiding this comment.
Please don't split lines.
| resource "github_enterprise_security_configuration" "test" { | ||
| enterprise_slug = "%s" | ||
| name = "%s" | ||
| description = "Test configuration" | ||
| advanced_security = "enabled" | ||
| dependency_graph = "enabled" | ||
| dependabot_alerts = "enabled" | ||
| dependabot_security_updates = "enabled" | ||
| code_scanning_default_setup = "enabled" | ||
| secret_scanning = "enabled" | ||
| secret_scanning_push_protection = "enabled" | ||
| private_vulnerability_reporting = "enabled" | ||
| enforcement = "enforced" | ||
| }`, testAccConf.enterpriseSlug, configName) |
There was a problem hiding this comment.
| resource "github_enterprise_security_configuration" "test" { | |
| enterprise_slug = "%s" | |
| name = "%s" | |
| description = "Test configuration" | |
| advanced_security = "enabled" | |
| dependency_graph = "enabled" | |
| dependabot_alerts = "enabled" | |
| dependabot_security_updates = "enabled" | |
| code_scanning_default_setup = "enabled" | |
| secret_scanning = "enabled" | |
| secret_scanning_push_protection = "enabled" | |
| private_vulnerability_reporting = "enabled" | |
| enforcement = "enforced" | |
| }`, testAccConf.enterpriseSlug, configName) | |
| resource "github_enterprise_security_configuration" "test" { | |
| enterprise_slug = "%s" | |
| name = "%s" | |
| description = "Test configuration" | |
| advanced_security = "enabled" | |
| dependency_graph = "enabled" | |
| dependabot_alerts = "enabled" | |
| dependabot_security_updates = "enabled" | |
| code_scanning_default_setup = "enabled" | |
| secret_scanning = "enabled" | |
| secret_scanning_push_protection = "enabled" | |
| private_vulnerability_reporting = "enabled" | |
| enforcement = "enforced" | |
| }`, testAccConf.enterpriseSlug, configName) |
Please use Terraform formatting (2-space) not Go formatting.
| tmpl := ` | ||
| resource "github_enterprise_security_configuration" "test" { | ||
| enterprise_slug = "%s" | ||
| name = "%s" | ||
| description = "%s" | ||
| advanced_security = "%s" | ||
| }` | ||
| configBefore := fmt.Sprintf(tmpl, testAccConf.enterpriseSlug, configName, "Test configuration", "disabled") | ||
| configAfter := fmt.Sprintf(tmpl, testAccConf.enterpriseSlug, configNameUpdated, "Test configuration updated", "enabled") |
There was a problem hiding this comment.
Tests are easier to reason about if config is pre-rendered with the fixed values (%v) while keeping placeholders (%%v) so Config: fmt.Sprintf(config, myValue),.
|
|
||
| // configuration_id must be identical across both steps, proving the change is an | ||
| // in-place update rather than a destroy-and-recreate. | ||
| configurationIDUnchanged := statecheck.CompareValue(compare.ValuesSame()) |
There was a problem hiding this comment.
Please use ConfigPlanChecks to check the resource action rather than checking the ID isn;t changed.
This commit adds a new resource github_organization_security_configuration & github_enterprise_security_configuration to manage Code Security Configurations at the organization & enterprise level respectively. It includes:
Resolves #2412
Before the change?
After the change?
Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!
Tests