From a37ca29667b1cd6c95619e636348ba9c4143450c Mon Sep 17 00:00:00 2001 From: NWarila <33955773+NWarila@users.noreply.github.com> Date: Tue, 26 May 2026 13:16:55 +0000 Subject: [PATCH] feat(repo-schema): manage allow_forking with ownership-aware default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The GitHub API rejects any PATCH containing `allow_forks` on personal-account private repositories — regardless of the value being set. terraform-provider-github includes `allow_forks` in every PATCH payload for `github_repository` resources by default, so deploys that manage personal-account private repos always failed with 422 "Allow forks setting can only be changed on org-owned private repositories", even though the framework didn't intentionally expose the field in YAML. This change makes `allow_forking` an opt-in YAML key with a default policy that handles all three GitHub API behaviors: YAML explicitly sets true/false: Honor the value verbatim. Lets repos that need a specific fork policy declare it. YAML omits the key + visibility == "private" + var.github_is_organization == false: Default null. The provider then omits `allow_forks` from PATCH, bypassing the API rejection for personal-account private repos. Every other case (public, internal, OR org-owned private): Default false (secure-by-default; forks disabled unless YAML opts in via `allow_forking: true`). GitHub's API accepts the field on all three, so the framework can enforce the policy. Concretely: - terraform/30-locals.tf `allow_forking` added to `allowed_repo_keys` (was rejected as unknown-top-level-key before). Build expression in `local.all_repositories` implements the three-way logic above. - terraform/41-resources-github.tf Resource now explicitly assigns `allow_forking = each.value.allow_forking`. Removed `allow_forking` from the lifecycle `ignore_changes` list — drift in YAML-managed values should surface as a real diff. - terraform/tests/validation.tftest.hcl Removed `rejects_allow_forking` test (the framework now accepts the key). - terraform/tests/fixtures/bad-allow-forking/ (deleted) Fixture is no longer "bad" under the new schema. - terraform/tests/COVERAGE.md G03 row repurposed to track the new positive coverage (apply-time test against a synthetic provider is a follow-up). Renders correctly under both ownership values: - var.github_is_organization = false (personal-account): private repos default null → no PATCH rejection. - var.github_is_organization = true (org): private repos default false → framework enforces secure default. No DESIGN.md or README changes; the existing prose about rejecting unknown keys still describes the general typo-protection behavior, and `allow_forking` is now an allowed top-level key. A follow-up doc PR can add a paragraph about the ownership-aware default if a maintainer finds the YAML schema needs it. Co-Authored-By: Claude Opus 4.7 (1M context) --- terraform/30-locals.tf | 37 ++++++++++++++++--- terraform/41-resources-github.tf | 13 ++++--- terraform/tests/COVERAGE.md | 2 +- .../bad-allow-forking/private/.gitkeep | 0 .../fixtures/bad-allow-forking/public/bad.yml | 8 ---- terraform/tests/validation.tftest.hcl | 12 ------ 6 files changed, 40 insertions(+), 32 deletions(-) delete mode 100644 terraform/tests/fixtures/bad-allow-forking/private/.gitkeep delete mode 100644 terraform/tests/fixtures/bad-allow-forking/public/bad.yml diff --git a/terraform/30-locals.tf b/terraform/30-locals.tf index d9b8070..c252411 100644 --- a/terraform/30-locals.tf +++ b/terraform/30-locals.tf @@ -35,16 +35,18 @@ locals { # outside this set indicates a typo or schema drift and is rejected by # terraform_data.framework_validation. # - # NOTE: `allow_forking` is intentionally absent. The setting is org-only - # in the GitHub API and this framework does not currently manage it via a - # provider-backed resource (github_repository_setting / org-level config). - # Accepting the key would be a silent no-op, so it is rejected at the - # unknown-key stage with a clear error pointing at the offending repo. + # NOTE: `allow_forking` is opt-in. The GitHub API rejects PATCH calls + # that include the field on personal-account private repositories, so + # this framework treats the field as null-by-default. A repo YAML may + # explicitly opt in by setting `allow_forking: true|false` — the + # provider includes the field in PATCH only when non-null, so opt-in + # repos manage the setting normally and opt-out (default) repos never + # trip the API rejection. allowed_repo_keys = toset([ "description", "homepage_url", "topics", "fork", "source_owner", "source_repo", "visibility", "has_discussions", "has_issues", "has_projects", "has_wiki", "is_template", - "allow_auto_merge", "allow_merge_commit", "allow_rebase_merge", + "allow_auto_merge", "allow_forking", "allow_merge_commit", "allow_rebase_merge", "allow_squash_merge", "allow_update_branch", "delete_branch_on_merge", "merge_commit_message", "merge_commit_title", "squash_merge_commit_message", "squash_merge_commit_title", @@ -433,6 +435,29 @@ locals { local.repo_setting_defaults.allow_auto_merge ) + # Default policy (ownership-aware): + # 1. YAML explicitly sets true/false → honor it. + # 2. YAML omits the key + visibility == "private" + # + github_is_organization == false → null. + # (Personal-account private: the API rejects any PATCH + # containing allow_forks regardless of value, so the + # provider must omit the field entirely.) + # 3. Every other case → false (secure-by-default; forks disabled + # unless YAML opts in via `allow_forking: true`). + # Covers public, internal, AND org-owned private — + # GitHub's API accepts the field for all three, so the + # framework enforces the policy. + allow_forking = ( + try(repository.allow_forking, null) != null + ? repository.allow_forking + : ( + coalesce(try(repository.visibility, null), "private") == "private" + && !var.github_is_organization + ? null + : false + ) + ) + allow_merge_commit = coalesce( try(repository.allow_merge_commit, null), local.repo_setting_defaults.allow_merge_commit diff --git a/terraform/41-resources-github.tf b/terraform/41-resources-github.tf index c76eccc..190161b 100644 --- a/terraform/41-resources-github.tf +++ b/terraform/41-resources-github.tf @@ -49,6 +49,7 @@ resource "github_repository" "repo" { is_template = each.value.is_template # Merge Behavior + allow_forking = each.value.allow_forking allow_merge_commit = each.value.allow_merge_commit allow_squash_merge = each.value.allow_squash_merge allow_rebase_merge = each.value.allow_rebase_merge @@ -153,11 +154,13 @@ resource "github_repository" "repo" { # check repo-scoped invariants so their errors point at the specific # github_repository.repo[] address. lifecycle { - # `allow_forking` is an org-owned private/internal repository setting in - # GitHub's API. The provider may observe it during refresh, but this - # framework does not expose it in repo YAML and must not PATCH it for - # personal-account repositories. - ignore_changes = [auto_init, license_template, allow_forking] + # `auto_init` and `license_template` are CREATE-time only; ignoring + # them prevents spurious diff after the initial create. `allow_forking` + # is now explicitly managed in YAML (default null for private repos so + # the provider omits it from PATCH; default false for public/internal; + # YAML may override either). It is NOT ignored anymore — drift in + # YAML-managed values should surface as a real diff. + ignore_changes = [auto_init, license_template] precondition { condition = contains(["public", "private", "internal"], each.value.visibility) diff --git a/terraform/tests/COVERAGE.md b/terraform/tests/COVERAGE.md index ff65618..9b5ed01 100644 --- a/terraform/tests/COVERAGE.md +++ b/terraform/tests/COVERAGE.md @@ -36,7 +36,7 @@ terraform test |---|---|---|---| | G01 | Duplicate repo keys across `public/` and `private/` | ✅ | `validation.tftest.hcl::rejects_duplicate_repo_keys` | | G02 | Unknown top-level YAML key | ✅ | `validation.tftest.hcl::rejects_unknown_top_level_key` | -| G03 | `allow_forking` rejected as unknown key | ✅ | `validation.tftest.hcl::rejects_allow_forking` | +| G03 | `allow_forking` accepted and applied with visibility-aware default (private→null, else false; YAML may override) | ⚠️ | needs apply-time test against synthetic provider | | G04 | Unknown nested: `actions.*` (top) | ✅ | `validation.tftest.hcl::rejects_unknown_nested_key` + multi-typo | | G05 | Unknown nested: `actions.allowed_actions_config.*` | ✅ | `validation.tftest.hcl::rejects_multiple_nested_typos_in_one_repo` | | G06 | Unknown nested: `pages.*` | ✅ | `validation.tftest.hcl::rejects_multiple_nested_typos_in_one_repo` | diff --git a/terraform/tests/fixtures/bad-allow-forking/private/.gitkeep b/terraform/tests/fixtures/bad-allow-forking/private/.gitkeep deleted file mode 100644 index e69de29..0000000 diff --git a/terraform/tests/fixtures/bad-allow-forking/public/bad.yml b/terraform/tests/fixtures/bad-allow-forking/public/bad.yml deleted file mode 100644 index c1e6fe4..0000000 --- a/terraform/tests/fixtures/bad-allow-forking/public/bad.yml +++ /dev/null @@ -1,8 +0,0 @@ -bad-allow-forking-repo: - description: "allow_forking fixture" - visibility: private - # allow_forking is intentionally absent from local.allowed_repo_keys - # because the framework does not currently manage the setting. Accepting - # it would be a silent no-op, so it is rejected at the unknown-top-level - # key validation stage. - allow_forking: true diff --git a/terraform/tests/validation.tftest.hcl b/terraform/tests/validation.tftest.hcl index 7564f05..ec846ae 100644 --- a/terraform/tests/validation.tftest.hcl +++ b/terraform/tests/validation.tftest.hcl @@ -73,18 +73,6 @@ run "rejects_unknown_nested_key" { ] } -run "rejects_allow_forking" { - command = plan - - variables { - repo_yaml_path = "tests/fixtures/bad-allow-forking" - } - - expect_failures = [ - terraform_data.framework_validation, - ] -} - run "rejects_duplicate_repo_keys" { command = plan