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 |
sprioriello
changed the title
…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
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! |
main has moved go-github from v88 to v89 (integrations#89.0.0). The security configuration resources, helpers, and unit test were the only files still importing v88, which broke the build once the branch was merged with main (compile, CodeQL, strict linting, and docs generation all failed with "no required module provides package .../v88/github"). Bump the import path to v89 in all four files; the API surface used is unchanged. Build, vet, gofmt, golangci-lint (default + strict), and tfplugindocs are all clean, with no docs drift.
|
Thanks @stevehipwell — the automation failures were all one root cause: Fixed in sprioriello#10: merged @sprioriello once the PR is merged the branch will be current with |
fix: update branch to main and migrate security-config to go-github v89
|
Thanks again @casey-robertson-paypal — sprioriello#10 is now merged into @stevehipwell the CI / CodeQL (Analyze go) / strict-lint / docs failures you flagged traced back to that single v88→v89 root cause and are now resolved — the branch is green and mergeable. It's ready for another review whenever you have a moment. Thanks for the thorough passes! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Now that the branch builds on go-github v89, the strict "new code" lint and CodeQL analysis run for the first time and flag the security configuration code: - forcetypeassert: every unchecked type assertion on d.Get/d.GetOk and the flatten unit-test results. Switch to the comma-ok form used elsewhere in the repo, and collapse the repetitive optional-string expansion into a shared expandOptionalString helper. - CodeQL "incorrect integer conversion" (G115): the import functions parsed configuration_id with strconv.ParseInt (int64) then converted to int without a bound check. Use strconv.Atoi, which yields an int directly and matches the schema.TypeInt attribute. Build, vet, gofmt, unit tests, and golangci-lint (default, strict, and the generated new-code config) are all clean.
|
Thanks @stevehipwell — you were right that these validate locally. Now that the branch builds on go-github v89, the strict "new code" linter and CodeQL ran for the first time and surfaced two things, both fixed in sprioriello#11:
I reproduced CI locally by generating the same @sprioriello once sprioriello#11 is merged the remaining checks should go green. Sorry about all the iterations! So close. I'll do better to align future PRs with lessons learned from this one. |
…lint fix: resolve forcetypeassert and CodeQL integer-conversion CI failures
There was a problem hiding this comment.
Pull request overview
These provider review instructions are being used.
Adds organization- and enterprise-level resources for managing GitHub Code Security Configurations. Blocking findings involve incomplete imported state, unsupported runner_type = "not_set", and missing permission documentation.
Changes:
- Implements CRUD, import, schemas, and shared conversion helpers.
- Adds provider registration and automated tests.
- Adds examples, import commands, templates, and generated documentation.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
github/resource_github_organization_security_configuration.go |
Implements the organization resource. |
github/resource_github_organization_security_configuration_test.go |
Adds organization acceptance tests. |
github/resource_github_enterprise_security_configuration.go |
Implements the enterprise resource. |
github/resource_github_enterprise_security_configuration_test.go |
Adds enterprise acceptance tests. |
github/util_security_configuration.go |
Adds shared flattening helpers. |
github/util_security_configuration_test.go |
Tests shared conversion behavior. |
github/provider.go |
Registers both resources. |
templates/resources/organization_security_configuration.md.tmpl |
Adds organization documentation template. |
templates/resources/enterprise_security_configuration.md.tmpl |
Adds enterprise documentation template. |
docs/resources/organization_security_configuration.md |
Adds generated organization documentation. |
docs/resources/enterprise_security_configuration.md |
Adds generated enterprise documentation. |
examples/resources/github_organization_security_configuration/resource_1.tf |
Demonstrates organization usage. |
examples/resources/github_organization_security_configuration/import.sh |
Demonstrates CLI import. |
examples/resources/github_organization_security_configuration/import-by-string-id.tf |
Demonstrates declarative import. |
examples/resources/github_enterprise_security_configuration/resource_1.tf |
Demonstrates enterprise usage. |
examples/resources/github_enterprise_security_configuration/import.sh |
Demonstrates enterprise CLI import. |
examples/resources/github_enterprise_security_configuration/import-by-string-id.tf |
Demonstrates enterprise declarative import. |
|
@stevehipwell Copilot's review here is advisory — a quick triage, deferring to your guidance on all of it:
Glad to push a small follow-up for the last two if you want them in this PR, or leave them for later — your call. Worth noting all CI checks are green now. |
Addresses the remaining Copilot review comments on integrations#3284. code_scanning_default_setup_options.runner_type rejected "not_set", which the REST API accepts for both the organization and enterprise endpoints, so there was no way to reset a previously configured runner selection: removing the block only omits it from the PATCH. Widen the validation to include it. go-github declares RunnerType as a plain string with `json:"runner_type"` and no omitempty, so declaring the block without runner_type serialised "runner_type": "" and the API rejected the request. Default to "not_set" so that path is valid. The doc templates named only "organization admin" / "enterprise admin" access and no token scope. The org endpoints also permit security managers and need write:org for classic PATs; the enterprise endpoints need admin:enterprise. State the role and scope and link to the REST reference for the full list, per the "Permissions and Scopes" guidance in docs.instructions.md. Docs regenerated with tfplugindocs.
|
@casey-robertson-paypal thanks for the triage — took your read on it and pushed
Token scopes / security managers — both templates now name the role and classic-PAT scope ( Import / CI/CodeQL are in |
…ecurity-configuration
|
@stevehipwell this is ready for another review when you have a moment. Since your last pass:
Build, vet, unit tests, One thing that needs your call, on the two review threads I left unresolved: Copilot wants Also, CI and CodeQL are sitting in |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.
Suppressed comments (4)
github/resource_github_organization_security_configuration.go:485
github.CodeSecurityConfiguration.Descriptionis a non-pointer field tagged asjson:"description", so this conditional does not omit the field: an unset Terraform value is still sent as"description":"". Updating any other setting after import or out-of-band edits will therefore erase a remote description that this Optional-only design otherwise treats as unmanaged. Preserve the current description when it is absent from configuration, or use a PATCH payload that can truly omit it.
if val, ok := d.GetOk("description"); ok {
config.Description, _ = val.(string)
}
github/resource_github_enterprise_security_configuration.go:435
github.CodeSecurityConfiguration.Descriptionis a non-pointer field tagged asjson:"description", so this conditional does not omit the field: an unset Terraform value is still sent as"description":"". Updating any other setting after import or out-of-band edits will therefore erase a remote description that this Optional-only design otherwise treats as unmanaged. Preserve the current description when it is absent from configuration, or use a PATCH payload that can truly omit it.
if val, ok := d.GetOk("description"); ok {
config.Description, _ = val.(string)
}
github/resource_github_organization_security_configuration_test.go:90
- These API-facing acceptance scenarios only assert
configuration_id, so they would still pass if the configured security settings and nested options were dropped by expansion or refresh. Add state checks for representative scalar and nested values, plus an update step that verifies those values round-trip; the delegated-bypass scenario should also assert its reviewer fields.
Config: config,
ConfigStateChecks: []statecheck.StateCheck{
statecheck.ExpectKnownValue("github_organization_security_configuration.test", tfjsonpath.New("configuration_id"), knownvalue.NotNull()),
},
github/resource_github_enterprise_security_configuration_test.go:92
- These API-facing acceptance scenarios only assert
configuration_id, so they would still pass if the configured security settings and nested options were dropped by expansion or refresh. Add state checks for representative scalar and nested values and an update step that verifies those values round-trip.
Config: config,
ConfigStateChecks: []statecheck.StateCheck{
statecheck.ExpectKnownValue("github_enterprise_security_configuration.test", tfjsonpath.New("configuration_id"), knownvalue.NotNull()),
},
|
@stevehipwell Good day Steve! Anything myself or @sprioriello can do to help move this one forward? Thanks |
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