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
github/resource_github_organization_security_configuration_test.go
Outdated
Show resolved
Hide resolved
github/resource_github_organization_security_configuration_test.go
Outdated
Show resolved
Hide resolved
…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. |
| configAfter := fmt.Sprintf(` | ||
| resource "github_enterprise_security_configuration" "test" { | ||
| enterprise_slug = "%s" | ||
| name = "%s" | ||
| description = "Test configuration updated" | ||
| advanced_security = "enabled" | ||
| }`, testAccConf.enterpriseSlug, configNameUpdated) |
There was a problem hiding this comment.
Please use a single template string
There was a problem hiding this comment.
Done. Consolidated into a single template string with placeholders for the varying fields (name, description, advanced_security).
| id, err := buildID(orgName, configIDStr) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| d.SetId(id) |
There was a problem hiding this comment.
This seems superfluous, since d.Id() is already that exact string
There was a problem hiding this comment.
Removed the superfluous buildID/SetId round-trip in both org and enterprise import functions. The org import now blanks the org name with _ since it is obtained from meta.(*Owner).name in CRUD operations (consistent with other org-level resources) in the repo.
There was a problem hiding this comment.
Please use testing arrays to improve test maintainability in the future
There was a problem hiding this comment.
Refactored all four TestFlatten* functions to use table-driven test arrays with tests := []struct{...} and for _, tt := range tests, matching the pattern in util_ruleset_validation_test.go.
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.
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