feat: Add github_repository_pull_request_creation_policy resource - #3349
feat: Add github_repository_pull_request_creation_policy resource#3349RoFz wants to merge 11 commits into
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 |
6f14748 to
4d4fe11
Compare
deiga
left a comment
There was a problem hiding this comment.
The github_repository resource relies solely on the REST API currently. If you need to use GQL API, then please do that in a separate resource.
- refactor: extract pull_request_creation_policy into github_repository_pull_request_creation_policy resource - refactor: revert pull_request_creation_policy field and GraphQL calls from github_repository - refactor: remove isUnsupportedPullRequestCreationPolicyError; dedicated resource fails loudly on missing API field - feat: add github_repository_pull_request_creation_policy resource with GraphQL CRUD and import - test: add acceptance tests for github_repository_pull_request_creation_policy - docs: add documentation for github_repository_pull_request_creation_policy
4d4fe11 to
efeb9d4
Compare
RoFz
left a comment
There was a problem hiding this comment.
Thanks for the review. Reworked to a dedicated github_repository_pull_request_creation_policy resource that uses only the GraphQL API, keeping github_repository REST-only.
- chore(merge): integrate upstream main into pull request creation policy branch - fix(provider): register github_repository_pull_request_creation_policy after provider.go reformat - docs(repository_pull_request_creation_policy): migrate doc to tfplugindocs template, example, and generated output
|
Merged the latest 1.
2. Documentation layout migration
The argument reference rows come from the resource's schema Verification
|
…nt destroy behavior - fix(repository_pull_request_creation_policy): remove resource from state when the repository no longer exists - fix(repository_pull_request_creation_policy): error on an empty pull request creation policy read instead of producing a perpetual diff - refactor(repository_pull_request_creation_policy): extract isRepositoryNotFoundError helper for the GraphQL not-found check - test(repository_pull_request_creation_policy): cover empty policy mapping, the not-found helper, and the real GraphQL not-found wire format - docs(repository_pull_request_creation_policy): document that destroy resets the policy to all
|
Follow-up to the conflict resolution above. While re-reviewing the resource I also pushed a few robustness and docs improvements (commit b2f15e8). These were not requested in the earlier review, so if you'd rather keep this PR limited to the new resource and the conflict fix, I'm happy to pull them into a separate follow-up PR. What changed:
On the not-found detection: the v4 client ( Verification: |
| }, | ||
|
|
||
| Schema: map[string]*schema.Schema{ | ||
| "repository": { |
There was a problem hiding this comment.
Please add also repository_id and the diffRepo CustomizeDiff configuration to support renaming repositories
There was a problem hiding this comment.
Done in 97d8104: removed ForceNew from repository, added a computed repository_id, and set CustomizeDiff: diffRepository, per the "Repository as a Required Argument" convention. An acceptance subtest renames the repository and asserts the resource is updated in place rather than replaced, with the policy preserved.
| } | ||
|
|
||
| d.SetId(repoName) | ||
| return resourceGithubRepositoryPullRequestCreationPolicyRead(ctx, d, meta) |
There was a problem hiding this comment.
Never call any of the CRUD functions directly. If you need to set Computed fields, set them in the same function
There was a problem hiding this comment.
Done in 33377d3: Create and Update now return nil directly. One observation for context: the codebase currently has around 80 Create/Update implementations that end by calling Read (including github_branch_protection, which this resource was modeled on), while ARCHITECTURE.md and the schema-and-state instructions document the no-read-after-write convention from #2892. This resource now follows the documented convention, and the legacy call sites may be worth a tracking issue if a migration is planned. Computed fields are set in Create and Update directly (see the repository_id change for rename support).
| if err != nil { | ||
| return diag.Errorf("error reading pull request creation policy for %s/%s: %s", owner, repoName, err) | ||
| } |
There was a problem hiding this comment.
Please differentiate between an actual error and for example the "repo doesn't exist anymore" case where this resource can be removed from state directly
There was a problem hiding this comment.
Done in b2f15e8: Read now detects the repository-not-found GraphQL error and removes the resource from state instead of failing. The detection lives in an isRepositoryNotFoundError helper with a unit test that pins GitHub's actual not-found payload, and the log uses structured tflog.
| repoName := d.Id() | ||
| policy := d.Get("policy").(string) | ||
|
|
||
| nodeID, err := getRepositoryID(repoName, meta) |
There was a problem hiding this comment.
Instead of wasting API calls on fetching the repo Node ID in each CRUD function, could we add a computed field to the schema to store it?
There was a problem hiding this comment.
Addressed in 97d8104. Create and Update now resolve the repository through a single by-name GraphQL query that returns both the node ID and the numeric ID, replacing the previous helper that probed the input as a node ID twice before falling back to the name. One note for transparency: the stored repository_id is the numeric ID required by diffRepository, not the node ID the mutation needs, so each write still resolves the node ID, but now in one query instead of three calls.
| if err != nil { | ||
| return diag.Errorf("error resolving repository node ID for %s: %s", repoName, err) | ||
| } | ||
|
|
||
| if err := updateRepositoryPullRequestCreationPolicy(ctx, nodeID, "all", meta); err != nil { | ||
| return diag.Errorf("error resetting pull request creation policy for %s: %s", repoName, err) | ||
| } |
There was a problem hiding this comment.
Please check if it's an actual error before returning an error. Is the repo gone? Not an actual error, delete resource from state and be happy.
There was a problem hiding this comment.
Done in 890cab5: Delete treats a missing repository as success. The guard sits on the by-name repository resolution, since that is where a missing repo surfaces with the error string the helper verifies. I did not guard the subsequent node-ID mutation: GitHub returns a different error shape there ("Could not resolve to a node with the global id"), it is only reachable in a delete-between-calls race, and a retry converges through the by-name guard anyway.
| diags := resourceGithubRepositoryPullRequestCreationPolicyRead(ctx, d, meta) | ||
| if diags.HasError() { | ||
| return nil, fmt.Errorf("%s", diags[0].Summary) | ||
| } |
There was a problem hiding this comment.
There is no need to call Read inside Import, the lifecycle works so that the next call will be Read anyway
There was a problem hiding this comment.
Done in 890cab5: replaced the custom importer with schema.ImportStatePassthroughContext. Read derives repository from the ID, so the custom importer added nothing, and ImportStateVerify passes against the live API.
| initial := `policy = "collaborators_only"` | ||
| updated := `policy = "all"` |
There was a problem hiding this comment.
It's clearer if you use the variables just for the policy values and not the whole property and value pair
There was a problem hiding this comment.
Done in f9a3ed3. Variables now hold just the policy values; the policy attribute lives in the config template.
| } | ||
| `, repoName) | ||
|
|
||
| checks := map[string]resource.TestCheckFunc{ |
There was a problem hiding this comment.
Don't use variables for this, inline it
There was a problem hiding this comment.
Done in f9a3ed3. The checks map is gone; each step carries its assertions inline.
| Steps: []resource.TestStep{ | ||
| { | ||
| Config: fmt.Sprintf(config, initial), | ||
| Check: checks["before"], |
There was a problem hiding this comment.
Please use ConfigStateChecks instead
There was a problem hiding this comment.
Done in f9a3ed3. Both subtests now use ConfigStateChecks with knownvalue matchers, and the suite (including create/update, import, and the rename scenario) passes against the live API.
There was a problem hiding this comment.
Pull request overview
These provider review instructions are being used.
This PR adds a new github_repository_pull_request_creation_policy resource that manages the pull request creation policy for a repository via the GitHub GraphQL API. It supports all and collaborators_only policy values and includes import support by repository name.
Changes:
- New resource implementation with full CRUD lifecycle using GraphQL, plus shared utility functions for querying/mutating the
pullRequestCreationPolicyfield - Unit tests for policy mapping functions and GraphQL interactions, plus acceptance tests covering create → update and import flows
- Documentation template, generated docs, and tfplugindocs example snippet
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
github/resource_github_repository_pull_request_creation_policy.go |
New resource with CRUD+import handling |
github/util_v4_repository.go |
Shared GraphQL helpers: query/mutation functions, enum types, isRepositoryNotFoundError |
github/util_v4_repository_test.go |
Unit tests for flatten/expand, GraphQL mocks, and not-found detection |
github/resource_github_repository_pull_request_creation_policy_test.go |
Acceptance tests for create/update and import |
github/provider.go |
Resource registration |
examples/resources/repository_pull_request_creation_policy/example_1.tf |
tfplugindocs example snippet |
templates/resources/repository_pull_request_creation_policy.md.tmpl |
Docs template |
docs/resources/repository_pull_request_creation_policy.md |
Generated documentation |
| func resourceGithubRepositoryPullRequestCreationPolicyDelete(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { | ||
| repoName := d.Id() | ||
|
|
||
| nodeID, err := getRepositoryID(repoName, meta) | ||
| if err != nil { | ||
| return diag.Errorf("error resolving repository node ID for %s: %s", repoName, err) | ||
| } | ||
|
|
||
| if err := updateRepositoryPullRequestCreationPolicy(ctx, nodeID, "all", meta); err != nil { | ||
| return diag.Errorf("error resetting pull request creation policy for %s: %s", repoName, err) | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Resolved in 890cab5. Delete returns nil when the repository no longer exists, guarding on the by-name resolution that matches the error path described here.
| Schema: map[string]*schema.Schema{ | ||
| "repository": { | ||
| Type: schema.TypeString, | ||
| Required: true, | ||
| ForceNew: true, | ||
| Description: "The name of the GitHub repository.", | ||
| }, | ||
| "policy": { | ||
| Type: schema.TypeString, | ||
| Required: true, | ||
| Description: "Controls who can create pull requests for the repository. Can be `all` or `collaborators_only`.", | ||
| ValidateDiagFunc: validation.ToDiagFunc(validation.StringInSlice([]string{"all", "collaborators_only"}, false)), | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Resolved in 97d8104. Added the computed repository_id, CustomizeDiff: diffRepository, and removed ForceNew, with an acceptance test covering the rename-in-place behavior.
… with provider conventions fix(repository_pull_request_creation_policy): treat a missing repository as success on delete refactor(repository_pull_request_creation_policy): use ImportStatePassthroughContext instead of a custom importer calling Read refactor(repository_pull_request_creation_policy): use structured tflog logging instead of log.Printf
…s in acceptance tests test(repository_pull_request_creation_policy): rewrite assertions with ConfigStateChecks and knownvalue matchers test(repository_pull_request_creation_policy): scope test variables to policy values and inline per-step checks
…ite in create and update refactor(repository_pull_request_creation_policy): return nil from create and update per the no-read-after-write convention
…ame without replacement feat(repository_pull_request_creation_policy): add computed repository_id and diffRepository so renaming a repository updates in place instead of forcing replacement refactor(repository_pull_request_creation_policy): resolve repository node and database ID in a single GraphQL query to avoid node-ID probe calls test(repository_pull_request_creation_policy): cover repository_id and a repository-rename acceptance scenario docs(repository_pull_request_creation_policy): document repository_id and rename-safe behavior
|
Is this superseded by #3479? |
yes, that one uses the SDK |
Resolves #3348
Before the change?
github_repositorycould not managepull_request_creation_policy.After the change?
github_repository_pull_request_creation_policyresource that manages the pull request creation policy for a repository via the GitHub GraphQL API.allandcollaborators_onlyvalues.Why a separate resource?
github_repositoryuses only the REST API. BecausepullRequestCreationPolicyis only exposed via GraphQL, this change follows the existing pattern used by other GraphQL-backed resources in this provider (e.g.github_team_settings,github_branch_protection) and introduces a dedicated resource rather than mixing transport layers ingithub_repository.Evidence
go test ./github -run 'TestPullRequestCreationPolicyMapping|TestRepositoryPullRequestCreationPolicyGraphQL' -count=1make testGH_TEST_AUTH_MODE=individual GITHUB_OWNER=RoFz GITHUB_USERNAME=RoFz make testacc T='TestAccGithubRepositoryPullRequestCreationPolicy'make lintchecktf-pr-creation-policy-manual-20260419-120606:terraform applysucceeded, GitHub API readback returnedcollaborators_only, and a follow-upterraform plan -detailed-exitcodereturned no changes.Pull request checklist
Does this introduce a breaking change?