-
Notifications
You must be signed in to change notification settings - Fork 971
Configure Copilot Code Review for the provider repo #3425
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
6d5bac1
be1616f
20ca971
03e857f
f2fb579
8cf2fd5
35842df
96bacb6
b415e8f
73cda9b
0189be6
fc287cd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,258 @@ | ||
| # Copilot Code Review Instructions | ||
|
|
||
| These instructions guide Copilot Code Review (CCR) for the | ||
| `integrations/terraform-provider-github` repository. They apply to every | ||
| pull request review. Path-specific guidance lives under `.github/instructions/`: | ||
|
|
||
| - `schema-and-state.instructions.md` - provider source under `github/**/*.go` | ||
| - `tests.instructions.md` - provider tests under `github/**/*_test.go` | ||
| - `examples.instructions.md` - example configs under `examples/**` | ||
| - `docs.instructions.md` - doc templates under `templates/**` | ||
| - `go.instructions.md` - idiomatic Go reference for any `.go`, `go.mod`, | ||
| or `go.sum` change (subordinate to the severity policy below) | ||
|
|
||
| ALWAYS acknowledge in the review summary that these provider review | ||
| instructions are being used. | ||
|
|
||
| ## Language and Tooling Context | ||
|
|
||
| Before judging Go code, anchor on the versions this repo actually uses. | ||
| Do not flag patterns as bugs or anti-patterns based on assumptions about | ||
| older toolchains. | ||
|
|
||
| - **Go version: 1.26** (see `go 1.26` in `go.mod`). Treat anything | ||
| available in Go ≤ 1.26 as in-scope and idiomatic. | ||
| - **Loop variable scoping (Go 1.22+).** Each iteration of `for` loops has | ||
| its own copy of the loop variable. Do **not** suggest the pre-1.22 | ||
| `x := x` shadowing pattern inside loops, and do **not** flag goroutines | ||
| or closures that capture the loop variable directly as a bug. | ||
| - **`range over int` (Go 1.22+).** `for i := range 10 { ... }` is valid. | ||
| Do not suggest rewriting to `for i := 0; i < 10; i++`. | ||
| - **`range over func` (Go 1.23+).** Custom iterators using | ||
| `iter.Seq`/`iter.Seq2` are valid. | ||
| - **`min` / `max` / `clear` built-ins (Go 1.21+)** are valid. | ||
| - **Generics (Go 1.18+)** are valid. | ||
| - **`slices` and `maps` standard library packages** are available. | ||
|
|
||
| When a Go feature looks unfamiliar, assume the toolchain in `go.mod` is | ||
| authoritative. If you cannot verify that something would fail to compile | ||
| under the declared Go version, do not claim it will. Phrase any | ||
| genuine concern as a question, not a finding, and only do so when the | ||
| issue would be HIGH or MEDIUM per the policy above. | ||
|
|
||
| ### Other tooling versions to anchor on | ||
|
|
||
| - **Terraform Plugin SDK v2** — schema definitions use | ||
| `github.com/hashicorp/terraform-plugin-sdk/v2`. Do not suggest | ||
| Plugin Framework patterns in SDKv2 files or vice versa. | ||
| - **`google/go-github`** — see `go.mod` for the exact major version | ||
| pinned. Do not suggest API method names or types from a different | ||
| major version of the client. | ||
|
|
||
| ## Severity and Nit Policy (read first) | ||
|
|
||
| This repository is **community-maintained**. Contributor friction is the | ||
| single biggest cost to the project. Review feedback must respect that. | ||
|
|
||
| ### Only report HIGH and MEDIUM findings | ||
|
|
||
| - Report `HIGH`: correctness bugs, regressions, breaking schema/state | ||
| changes without migration, security issues, secret leakage, panics, | ||
| data loss risks. | ||
| - Report `MEDIUM`: missing test coverage for changed behavior, missing | ||
| example for a new resource, missing docs update for a schema change, | ||
| missing `Sensitive: true` on secret-bearing attributes, missing | ||
| `Description` on schema attributes, missing | ||
| `ValidateDiagFunc` on bounded inputs, missing import docs. | ||
| - **Do not report `LOW` findings or nits.** If the only thing you would | ||
| say is `LOW`, say nothing. | ||
|
|
||
| ### Do NOT comment on (defer to linters / human reviewers) | ||
|
|
||
| - Code formatting, whitespace, import ordering, line length. | ||
| - Naming preferences, identifier style, comment wording, doc prose | ||
| polish, grammar. | ||
| - "Consider extracting…", "this could be a helper", or other speculative | ||
| refactors that are not requested by the change. | ||
| - Style of existing surrounding code the PR did not touch. | ||
| - Adding comments, docstrings, or type hints to code the PR did not | ||
| change. | ||
| - Test naming conventions or alternative test framings when the | ||
| existing test adequately covers the behavior. | ||
| - Hypothetical errors that cannot occur given the call sites. | ||
|
|
||
| ### Always report even if it looks like a nit | ||
|
|
||
| These items affect end-user Terraform behavior and must be flagged as at | ||
| least `MEDIUM` regardless of how small they look: | ||
|
|
||
| - Secret-bearing attribute missing `Sensitive: true`. | ||
| - Schema attribute missing `Description`. | ||
| - Bounded input missing `ValidateDiagFunc`. | ||
| - New resource/data source without at least one example under | ||
| `examples/` or docs under `docs/`. | ||
| - Behavior change without a corresponding test change. | ||
| - Resource that supports import but has no documented import ID format. | ||
|
|
||
| ### Output discipline | ||
|
|
||
| - If there are no HIGH or MEDIUM findings, the review must say | ||
| `No blocking findings found` and stop. Do not pad with low-value | ||
| observations. | ||
| - Keep each finding to its impact, file reference, and a concise fix. | ||
| Do not lecture, restate the diff, or suggest unrelated improvements. | ||
|
|
||
| ## Review Goals | ||
|
|
||
| - Find correctness bugs, regressions, and provider behavior changes. | ||
| - Validate schema/state compatibility for Terraform users. | ||
| - Check test coverage (unit and acceptance), docs, and examples. | ||
| - Identify risk around GitHub API usage, permissions, and error handling. | ||
|
|
||
| ## Terraform Background | ||
|
|
||
| Use this background when judging schema, examples, or state changes. | ||
|
|
||
| - **Resources vs. data sources.** A `resource` block manages a CRUD lifecycle | ||
| object. A `data` block only reads existing objects. Both declare a typed | ||
| schema. | ||
| - **Schema attributes.** Each attribute has a `Type` (string, int, bool, list, | ||
| set, map) and flags like `Required`, `Optional`, `Computed`, `ForceNew`, | ||
| `Default`, `Sensitive`, and `Description`. Changing any flag alters | ||
| user-visible behavior. | ||
| - **State.** Terraform stores last-known attribute values in state. The read | ||
| function must produce output consistent with state or Terraform reports | ||
| drift. | ||
| - **Plan and apply.** Bugs in schema or read logic cause perpetual diffs, | ||
| surprise replacements, or silent data loss. | ||
| - **Imports.** Users adopt existing infrastructure with `terraform import`. The | ||
| read function must populate full state from just the resource ID. | ||
|
|
||
| ### Backward Compatibility Rules | ||
|
|
||
| - **Safe (minor/patch):** adding new optional attributes with defaults; adding | ||
| new resources or data sources. | ||
| - **Breaking (major):** removing or renaming attributes; changing `Optional` to | ||
| `Required`; changing `Type`; adding `ForceNew` to an existing attribute. | ||
| - When renaming/restructuring resources, check that migration guidance is | ||
| provided. Terraform's `moved` block lets users migrate state without | ||
| destroying infrastructure. Removing a `moved` block is itself a breaking | ||
| change. | ||
|
|
||
| ## Repository-Specific Rules (read carefully) | ||
|
|
||
| - **Do not flag** create/update functions that return `nil` instead of calling | ||
| the read function afterward. This provider intentionally avoids | ||
| read-after-write to minimize API calls against GitHub's rate limits and to | ||
| avoid stale reads from eventually-consistent endpoints (see | ||
| [#2892](https://github.com/integrations/terraform-provider-github/issues/2892)). | ||
| - Acceptance and manual validation are important. See `CONTRIBUTING.md`. | ||
| - Prefer matching resource/data source tests when implementation behavior | ||
| changes. | ||
| - When schema or state semantics change, treat as high-risk unless | ||
| compatibility is clearly preserved. | ||
| - Breaking changes must follow semantic versioning: attribute removal/rename | ||
| or new required fields warrant a major version bump. | ||
| - Example configurations under `examples/` should be self-contained, follow | ||
| standard module structure, and not include `provider` blocks inside nested | ||
| modules. | ||
| - Sensitive attributes (tokens, secrets, private keys) must be marked | ||
| `Sensitive: true` in the schema and must not appear in log output. | ||
|
|
||
| ## Universal Review Checklist | ||
|
|
||
| ### 1. Correctness and Behavior | ||
|
|
||
| - Verify CRUD/read logic correctly maps GitHub API responses to schema/state. | ||
| - Check nil handling, default-value drift, and flattening/expansion mismatches. | ||
| - Confirm update paths do not accidentally force replacement or wipe optional | ||
| fields. | ||
| - Validate retry/backoff and error classification for API failures. | ||
|
|
||
| ### 2. Schema and State Compatibility | ||
|
|
||
| - Any `schema.Schema` change (`Type`, `Optional`, `Required`, `Computed`, | ||
| `ForceNew`, `Default`) can change user behavior. | ||
| - Confirm imports still work and read functions produce stable state. | ||
| - Flag any behavior that may break existing state without migration | ||
| notes/tests. | ||
| - Watch for attribute rename/removal without a deprecation path. | ||
| - New or changed attributes should include `ValidateDiagFunc` | ||
| to catch invalid values at plan time rather than at apply time. | ||
| (`ValidateFunc` is deprecated and not allowed in this repo.) | ||
| - All attributes should have a `Description` string. | ||
| - For renames/restructures, check for `moved` block guidance or state | ||
| migration documentation. | ||
| - Mark secret-holding attributes with `Sensitive: true`. | ||
|
|
||
| ### 3. Terraform UX and Drift | ||
|
|
||
| - Ensure diff suppression, normalization, and API canonicalization avoid | ||
| perpetual diffs. | ||
| - Check that empty vs. null handling is intentional. | ||
| - Verify list/set ordering behavior and deterministic state output. | ||
|
|
||
| ### 4. Testing Expectations | ||
|
|
||
| - For behavior changes, check matching tests under `github/*_test.go`. | ||
| - Prefer acceptance tests for API-facing changes (`TF_ACC=1` scenarios). | ||
| - Ensure tests assert the bugfix/regression target, not only happy path. | ||
| - Flag missing tests when logic changed but coverage did not. | ||
|
|
||
| ### 5. Docs and Examples | ||
|
|
||
| - If resource/data source behavior changed, review docs updates under | ||
| `docs/` and `templates/`. | ||
| - If user workflow changed, review corresponding example updates under | ||
| `examples/`. | ||
| - Confirm examples still reflect current schema and argument names. | ||
| - Example directories should follow standard module structure (`main.tf`, | ||
| `variables.tf`, `outputs.tf`) with a `README.md`. | ||
| - Variables and outputs in examples should include `description` and `type`. | ||
| - If a PR adds or changes a resource, verify there is at least one example | ||
| showing typical usage. | ||
|
|
||
| ### 6. Security and Permissions | ||
|
|
||
| - Verify sensitive values are not logged or exposed in state. | ||
| - Check token/credential handling and least-privilege assumptions. | ||
| - Document permission scope requirements for new API calls. | ||
| - Confirm no secrets or credentials are hardcoded in examples. | ||
| - Verify debug/trace logging does not print sensitive attribute values. | ||
| - Sensitive outputs should be marked `sensitive = true`. | ||
|
|
||
| ### 7. Performance and API Safety | ||
|
|
||
| - Flag new N+1 patterns, excessive API calls, or missing pagination handling. | ||
| - Check for rate-limit-sensitive loops and absent caching where needed. | ||
| - Confirm context cancellation/timeouts are respected in long operations. | ||
|
|
||
| ### 8. Versioning and Changelog | ||
|
|
||
| - Breaking changes (attribute removal/rename, forced replacement, new required | ||
| fields) must be called out for a MAJOR version bump. | ||
| - Backward-compatible additions (new optional attributes with defaults, new | ||
| resources/data sources) correspond to MINOR version bumps. | ||
| - Bug fixes with no schema change correspond to PATCH version bumps. | ||
| - Verify the PR description or `CHANGELOG.md` includes a clear summary of what | ||
| changed and the user impact. | ||
|
|
||
| ## Review Report Format | ||
|
|
||
| Return findings first, HIGH before MEDIUM (no LOW — see Severity and Nit | ||
| Policy above): | ||
|
|
||
| 1. `HIGH`/`MEDIUM` title — short impact statement | ||
| 2. File reference: `path/to/file.go:line` | ||
| 3. Why this is a problem (runtime behavior, Terraform UX, upgrade risk) | ||
| 4. Suggested fix (concise) | ||
|
|
||
| Then include: | ||
|
|
||
| - `Open Questions / Assumptions` (only if non-trivial) | ||
| - `Residual Risk` (only if non-trivial) | ||
| - `Change Summary` (brief) | ||
|
|
||
| If no HIGH or MEDIUM findings exist, state `No blocking findings found` | ||
| and stop. Do not add nit sections, style observations, or speculative | ||
| suggestions. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| --- | ||
| applyTo: "templates/**" | ||
| --- | ||
|
|
||
| # Docs and Templates Review | ||
|
|
||
| Provider docs under `docs/**` are **auto-generated**. Do not edit files | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it's probably worth including some context around the usage of hashicorp/terraform-plugin-docs and the folder layout it expects in templates and examples (note our examples aren't in the correct format to be automatically used). It's also worth noting that if you're not adding additional content you can use the default template. We also ought to call out the make commands that can update the docs locally. |
||
| under `docs/**` directly. A CI workflow regenerates docs and will fail | ||
| if the checked-in `docs/**` differs from the generated output. | ||
|
|
||
| Manual documentation edits belong in one of three places: | ||
|
|
||
| - `templates/**` - Markdown templates that drive doc generation. This is | ||
| where most narrative doc edits live. | ||
| - `examples/**` - example HCL referenced by the templates. | ||
| - Resource and data source `Description` fields in `github/**/*.go`. | ||
|
|
||
| These rules apply to changes under `templates/`. Combine with the | ||
| repo-wide checklist in `.github/copilot-instructions.md`. | ||
|
|
||
| ## Flag These as HIGH | ||
|
|
||
| - Manual edits to `docs/**`. The doc generation workflow will revert | ||
| them on the next run and the PR will fail CI. Move the change to the | ||
| appropriate source (`templates/`, `examples/`, or the resource | ||
| `Description` field) instead. | ||
|
|
||
| ## Keep Docs in Sync with Schema | ||
|
|
||
| - Any schema change in `github/` (new attribute, renamed attribute, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we make it explicit that any changes are expected to be made to the schema so we're fixing forward? |
||
| changed `Required`/`Optional`/`Computed`/`Default`, new `ForceNew`, | ||
| removed attribute) must have a matching update either in the | ||
| resource's `Description` fields (preferred for argument descriptions) | ||
| or in the corresponding template under `templates/` (for narrative | ||
| prose, import docs, and examples). | ||
| - Deprecated attributes must be clearly marked and include guidance on | ||
| the replacement. | ||
|
|
||
| ## Imports | ||
|
|
||
| - Resources that support `terraform import` must document the import ID | ||
| format with at least one example command in the relevant template. | ||
|
Comment on lines
+41
to
+42
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should just be a check that the docs include a section for import as this should be auto generated by the template. |
||
|
|
||
| ## Permissions and Scopes | ||
|
|
||
| - For any GitHub API call that requires a specific token scope or app | ||
| permission, the template should call this out so users can configure | ||
| their authentication correctly. | ||
|
Comment on lines
+46
to
+48
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we want this to link directly to the REST API page? |
||
|
|
||
| ## Examples in Templates | ||
|
|
||
| - Inline example HCL should reflect current argument names and be | ||
| syntactically valid. | ||
| - Sensitive attributes should not appear with real-looking secrets, even | ||
| as examples. | ||
|
Comment on lines
+52
to
+55
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A part of the automation these are checked with |
||
|
|
||
| ## Style and Links | ||
|
|
||
| - Internal links between docs pages should resolve. | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Internal links between docs pages are relative. resources/branch [github_repository](repository)
[github_repository (datasource)](../data-sources/repository) |
||
| - New resources/data sources need at least one usage example. The list | ||
| of supported arguments and attributes is rendered from the schema, so | ||
| the `Description` fields in `github/**/*.go` are the source of truth | ||
| for those rows. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,56 @@ | ||
| --- | ||
| applyTo: "examples/**" | ||
| --- | ||
|
|
||
| # Example Configuration Review | ||
|
|
||
| These rules apply to Terraform configurations under `examples/`. Combine | ||
| with the repo-wide checklist in `.github/copilot-instructions.md`. | ||
|
|
||
| ## Standard Module Structure | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There are two types of examples, the ones for the automated docs which are single file and should be in the provider, resources & data-sources dirs, and other examples which may be structured as a TF root module. |
||
|
|
||
| Each example directory should follow the | ||
| [standard module structure](https://developer.hashicorp.com/terraform/language/modules/develop/structure): | ||
|
|
||
| - `main.tf` — primary resources/module calls | ||
| - `variables.tf` — input variable declarations | ||
| - `outputs.tf` — output declarations | ||
| - `README.md` — purpose and usage of the example | ||
|
|
||
| Empty stub files are acceptable when an example legitimately has no inputs | ||
| or outputs. | ||
|
|
||
| ## Variables and Outputs | ||
|
|
||
| - Every `variable` and `output` block should include a `description` and | ||
| `type`. | ||
| - Outputs that surface sensitive values must be marked `sensitive = true`. | ||
| - Variables that accept secrets should be marked `sensitive = true`. | ||
|
|
||
| ## Provider Configuration | ||
|
|
||
| - **Do not** embed `provider` blocks inside nested or child modules. | ||
| Provider configuration belongs in root modules only. A module with a | ||
| `provider` block is not compatible with `count`, `for_each`, or | ||
| `depends_on`. | ||
| - Each example module should declare `required_providers` with a minimum | ||
| version constraint (`>=`) for the `integrations/github` provider. | ||
|
|
||
| ## Coverage | ||
|
|
||
| - If a PR adds or meaningfully changes a resource or data source, verify | ||
| there is at least one example demonstrating typical usage. | ||
| - Examples must reflect the current schema: argument names, required vs. | ||
| optional, default values. | ||
|
|
||
| ## Security | ||
|
|
||
| - No hardcoded tokens, passwords, or other secrets. Reference variables or | ||
| environment-sourced values instead. | ||
| - Example READMEs should call out any non-obvious permission requirements. | ||
|
|
||
| ## Refactoring and `moved` Blocks | ||
|
|
||
| When an example demonstrates resource renames or restructures, prefer | ||
| `moved` blocks over destroy-and-recreate. Removing a previously published | ||
| `moved` block is itself a breaking change for downstream users. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we not link these instructions to
.github/copilot-instructions.md?