Skip to content

Commit 03e857f

Browse files
robert-crandallrobert-crandall
committed
Make review compatible with CCR
1 parent 20ca971 commit 03e857f

14 files changed

Lines changed: 368 additions & 2536 deletions

‎.github/copilot-instructions.md‎

Lines changed: 160 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,160 @@
1+
# Copilot Code Review Instructions
2+
3+
These instructions guide Copilot Code Review (CCR) for the
4+
`integrations/terraform-provider-github` repository. They apply to every
5+
pull request review. Path-specific guidance lives under `.github/instructions/`.
6+
7+
ALWAYS acknowledge in the review summary that these provider review
8+
instructions are being used.
9+
10+
## Review Goals
11+
12+
- Find correctness bugs, regressions, and provider behavior changes.
13+
- Validate schema/state compatibility for Terraform users.
14+
- Check test coverage (unit and acceptance), docs, and examples.
15+
- Identify risk around GitHub API usage, permissions, and error handling.
16+
17+
## Terraform Background
18+
19+
Use this background when judging schema, examples, or state changes.
20+
21+
- **Resources vs. data sources.** A `resource` block manages a CRUD lifecycle
22+
object. A `data` block only reads existing objects. Both declare a typed
23+
schema.
24+
- **Schema attributes.** Each attribute has a `Type` (string, int, bool, list,
25+
set, map) and flags like `Required`, `Optional`, `Computed`, `ForceNew`,
26+
`Default`, `Sensitive`, and `Description`. Changing any flag alters
27+
user-visible behavior.
28+
- **State.** Terraform stores last-known attribute values in state. The read
29+
function must produce output consistent with state or Terraform reports
30+
drift.
31+
- **Plan and apply.** Bugs in schema or read logic cause perpetual diffs,
32+
surprise replacements, or silent data loss.
33+
- **Imports.** Users adopt existing infrastructure with `terraform import`. The
34+
read function must populate full state from just the resource ID.
35+
36+
### Backward Compatibility Rules
37+
38+
- **Safe (minor/patch):** adding new optional attributes with defaults; adding
39+
new resources or data sources.
40+
- **Breaking (major):** removing or renaming attributes; changing `Optional` to
41+
`Required`; changing `Type`; adding `ForceNew` to an existing attribute.
42+
- When renaming/restructuring resources, check that migration guidance is
43+
provided. Terraform's `moved` block lets users migrate state without
44+
destroying infrastructure. Removing a `moved` block is itself a breaking
45+
change.
46+
47+
## Repository-Specific Rules (read carefully)
48+
49+
- **Do not flag** create/update functions that return `nil` instead of calling
50+
the read function afterward. This provider intentionally avoids
51+
read-after-write to minimize API calls against GitHub's rate limits and to
52+
avoid stale reads from eventually-consistent endpoints (see
53+
[#2892](https://github.com/integrations/terraform-provider-github/issues/2892)).
54+
- Acceptance and manual validation are important. See `CONTRIBUTING.md`.
55+
- Prefer matching resource/data source tests when implementation behavior
56+
changes.
57+
- When schema or state semantics change, treat as high-risk unless
58+
compatibility is clearly preserved.
59+
- Breaking changes must follow semantic versioning: attribute removal/rename
60+
or new required fields warrant a major version bump.
61+
- Example configurations under `examples/` should be self-contained, follow
62+
standard module structure, and not include `provider` blocks inside nested
63+
modules.
64+
- Sensitive attributes (tokens, secrets, private keys) must be marked
65+
`Sensitive: true` in the schema and must not appear in log output.
66+
67+
## Universal Review Checklist
68+
69+
### 1. Correctness and Behavior
70+
71+
- Verify CRUD/read logic correctly maps GitHub API responses to schema/state.
72+
- Check nil handling, default-value drift, and flattening/expansion mismatches.
73+
- Confirm update paths do not accidentally force replacement or wipe optional
74+
fields.
75+
- Validate retry/backoff and error classification for API failures.
76+
77+
### 2. Schema and State Compatibility
78+
79+
- Any `schema.Schema` change (`Type`, `Optional`, `Required`, `Computed`,
80+
`ForceNew`, `Default`) can change user behavior.
81+
- Confirm imports still work and read functions produce stable state.
82+
- Flag any behavior that may break existing state without migration
83+
notes/tests.
84+
- Watch for attribute rename/removal without a deprecation path.
85+
- New or changed attributes should include `ValidateFunc`/`ValidateDiagFunc`
86+
to catch invalid values at plan time rather than at apply time.
87+
- All attributes should have a `Description` string.
88+
- For renames/restructures, check for `moved` block guidance or state
89+
migration documentation.
90+
- Mark secret-holding attributes with `Sensitive: true`.
91+
92+
### 3. Terraform UX and Drift
93+
94+
- Ensure diff suppression, normalization, and API canonicalization avoid
95+
perpetual diffs.
96+
- Check that empty vs. null handling is intentional.
97+
- Verify list/set ordering behavior and deterministic state output.
98+
99+
### 4. Testing Expectations
100+
101+
- For behavior changes, check matching tests under `github/*_test.go`.
102+
- Prefer acceptance tests for API-facing changes (`TF_ACC=1` scenarios).
103+
- Ensure tests assert the bugfix/regression target, not only happy path.
104+
- Flag missing tests when logic changed but coverage did not.
105+
106+
### 5. Docs and Examples
107+
108+
- If resource/data source behavior changed, review website docs updates under
109+
`website/`.
110+
- If user workflow changed, review corresponding example updates under
111+
`examples/`.
112+
- Confirm examples still reflect current schema and argument names.
113+
- Example directories should follow standard module structure (`main.tf`,
114+
`variables.tf`, `outputs.tf`) with a `README.md`.
115+
- Variables and outputs in examples should include `description` and `type`.
116+
- If a PR adds or changes a resource, verify there is at least one example
117+
showing typical usage.
118+
119+
### 6. Security and Permissions
120+
121+
- Verify sensitive values are not logged or exposed in state.
122+
- Check token/credential handling and least-privilege assumptions.
123+
- Document permission scope requirements for new API calls.
124+
- Confirm no secrets or credentials are hardcoded in examples.
125+
- Verify debug/trace logging does not print sensitive attribute values.
126+
- Sensitive outputs should be marked `sensitive = true`.
127+
128+
### 7. Performance and API Safety
129+
130+
- Flag new N+1 patterns, excessive API calls, or missing pagination handling.
131+
- Check for rate-limit-sensitive loops and absent caching where needed.
132+
- Confirm context cancellation/timeouts are respected in long operations.
133+
134+
### 8. Versioning and Changelog
135+
136+
- Breaking changes (attribute removal/rename, forced replacement, new required
137+
fields) must be called out for a MAJOR version bump.
138+
- Backward-compatible additions (new optional attributes with defaults, new
139+
resources/data sources) correspond to MINOR version bumps.
140+
- Bug fixes with no schema change correspond to PATCH version bumps.
141+
- Verify the PR description or `CHANGELOG.md` includes a clear summary of what
142+
changed and the user impact.
143+
144+
## Review Report Format
145+
146+
Return findings first, ordered by severity:
147+
148+
1. `HIGH`/`MEDIUM`/`LOW` title — short impact statement
149+
2. File reference: `path/to/file.go:line`
150+
3. Why this is a problem (runtime behavior, Terraform UX, upgrade risk)
151+
4. Suggested fix (concise)
152+
153+
Then include:
154+
155+
- `Open Questions / Assumptions`
156+
- `Residual Risk`
157+
- `Change Summary` (brief)
158+
159+
If no issues are found, explicitly state `No blocking findings found` and list
160+
remaining risk areas.

‎.github/instructions/docs.instructions.md‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
---
2+
applyTo: "website/**"
3+
---
4+
5+
# Website / Docs Review
6+
7+
These rules apply to documentation under `website/`. Combine with the
8+
repo-wide checklist in `.github/copilot-instructions.md`.
9+
10+
## Keep Docs in Sync with Schema
11+
12+
- Any schema change in `github/` (new attribute, renamed attribute,
13+
changed `Required`/`Optional`/`Computed`/`Default`, new `ForceNew`,
14+
removed attribute) must have a matching docs update.
15+
- Argument tables should list attributes with the same name, type, and
16+
required/optional status as the schema.
17+
- Deprecated attributes must be clearly marked and include guidance on the
18+
replacement.
19+
20+
## Imports
21+
22+
- Resources that support `terraform import` must document the import ID
23+
format with at least one example command.
24+
25+
## Permissions and Scopes
26+
27+
- For any GitHub API call that requires a specific token scope or app
28+
permission, the docs should call this out so users can configure their
29+
authentication correctly.
30+
31+
## Examples in Docs
32+
33+
- Inline example HCL should reflect current argument names and be
34+
syntactically valid.
35+
- Sensitive attributes should not appear with real-looking secrets, even as
36+
examples.
37+
38+
## Style and Links
39+
40+
- Internal links between docs pages should resolve.
41+
- New resources/data sources need at least one usage example and a list of
42+
every supported argument and attribute.

‎.github/instructions/examples.instructions.md‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
---
2+
applyTo: "examples/**"
3+
---
4+
5+
# Example Configuration Review
6+
7+
These rules apply to Terraform configurations under `examples/`. Combine
8+
with the repo-wide checklist in `.github/copilot-instructions.md`.
9+
10+
## Standard Module Structure
11+
12+
Each example directory should follow the
13+
[standard module structure](https://developer.hashicorp.com/terraform/language/modules/develop/structure):
14+
15+
- `main.tf` — primary resources/module calls
16+
- `variables.tf` — input variable declarations
17+
- `outputs.tf` — output declarations
18+
- `README.md` — purpose and usage of the example
19+
20+
Empty stub files are acceptable when an example legitimately has no inputs
21+
or outputs.
22+
23+
## Variables and Outputs
24+
25+
- Every `variable` and `output` block should include a `description` and
26+
`type`.
27+
- Outputs that surface sensitive values must be marked `sensitive = true`.
28+
- Variables that accept secrets should be marked `sensitive = true`.
29+
30+
## Provider Configuration
31+
32+
- **Do not** embed `provider` blocks inside nested or child modules.
33+
Provider configuration belongs in root modules only. A module with a
34+
`provider` block is not compatible with `count`, `for_each`, or
35+
`depends_on`.
36+
- Each example module should declare `required_providers` with a minimum
37+
version constraint (`>=`) for the `integrations/github` provider.
38+
39+
## Coverage
40+
41+
- If a PR adds or meaningfully changes a resource or data source, verify
42+
there is at least one example demonstrating typical usage.
43+
- Examples must reflect the current schema: argument names, required vs.
44+
optional, default values.
45+
46+
## Security
47+
48+
- No hardcoded tokens, passwords, or other secrets. Reference variables or
49+
environment-sourced values instead.
50+
- Example READMEs should call out any non-obvious permission requirements.
51+
52+
## Refactoring and `moved` Blocks
53+
54+
When an example demonstrates resource renames or restructures, prefer
55+
`moved` blocks over destroy-and-recreate. Removing a previously published
56+
`moved` block is itself a breaking change for downstream users.

‎.github/instructions/schema-and-state.instructions.md‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,74 @@
1+
---
2+
applyTo: "github/**/*.go"
3+
---
4+
5+
# Provider Source Review (Schema, State, API)
6+
7+
These rules apply to all provider Go source files under `github/`. Combine
8+
with the repo-wide checklist in `.github/copilot-instructions.md`.
9+
10+
## Schema Changes Are User-Visible
11+
12+
Any change to `schema.Schema` (`Type`, `Optional`, `Required`, `Computed`,
13+
`ForceNew`, `Default`, `Sensitive`, `Description`) is potentially breaking.
14+
Flag all schema diffs and verify:
15+
16+
- Attribute removals or renames have a deprecation cycle or `moved`/state
17+
migration guidance.
18+
- `Optional` → `Required` transitions are called out as breaking.
19+
- New `ForceNew` flags on existing attributes are called out as breaking
20+
(forces resource replacement).
21+
- New attributes are `Optional` with a `Default` where reasonable to avoid
22+
forcing existing users to update their configs.
23+
- All attributes carry a `Description` string (used for docs generation).
24+
- Attributes accepting bounded values declare `ValidateFunc` or
25+
`ValidateDiagFunc` so invalid input fails at plan time, not apply time.
26+
- Attributes holding tokens, secrets, or private keys are marked
27+
`Sensitive: true`.
28+
29+
## State and Drift
30+
31+
- Read functions must populate every state attribute from API responses so
32+
`terraform import` works from the resource ID alone.
33+
- Verify the read path does not produce values that differ from what create/
34+
update wrote (causes perpetual diffs).
35+
- Watch list/set ordering: prefer `schema.TypeSet` or stable sort when the
36+
GitHub API does not return deterministic order.
37+
- Empty vs. null handling must be intentional and consistent between create,
38+
read, and update.
39+
- Diff suppression (`DiffSuppressFunc`) and normalization should be reviewed
40+
for correctness whenever schema is touched.
41+
42+
## CRUD Behavior
43+
44+
- Update paths must not accidentally force resource replacement or wipe
45+
optional fields that the user did not change.
46+
- Nil-check API response fields before dereferencing.
47+
- Classify errors: 404 from GitHub typically means "remove from state" in
48+
read; other errors should bubble up.
49+
- Respect `context.Context` cancellation and any configured timeouts.
50+
51+
## Repository-Specific: No Read-After-Write
52+
53+
**Do not flag** create or update functions that return `nil` instead of
54+
calling the resource's read function. This is intentional in this provider to
55+
minimize API calls against GitHub rate limits and to avoid stale reads from
56+
eventually-consistent endpoints. See
57+
[#2892](https://github.com/integrations/terraform-provider-github/issues/2892).
58+
59+
## API Safety and Performance
60+
61+
- Flag new N+1 access patterns over GitHub APIs.
62+
- Verify pagination is handled (`ListOptions` / `NextPage` loops) on any
63+
endpoint that returns a list.
64+
- Check for rate-limit-sensitive loops; consider caching or batching where
65+
appropriate.
66+
- Sensitive values must never appear in log output, even at debug/trace
67+
level.
68+
69+
## Security
70+
71+
- Token, credential, and webhook secret handling must follow least
72+
privilege.
73+
- New API calls should document the GitHub permission scope they require.
74+
- Do not hardcode secrets anywhere in source.

‎.github/instructions/tests.instructions.md‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
---
2+
applyTo: "github/**/*_test.go"
3+
---
4+
5+
# Provider Test Review
6+
7+
These rules apply to test files under `github/`. Combine with the repo-wide
8+
checklist in `.github/copilot-instructions.md`.
9+
10+
## Coverage Expectations
11+
12+
- When behavior in a resource or data source changes, there must be a
13+
matching test change. Flag PRs where production code changed but tests
14+
did not.
15+
- Prefer acceptance tests (`TF_ACC=1`) for any API-facing change. Unit-only
16+
coverage is rarely sufficient for schema or CRUD changes.
17+
- Tests should assert the specific bugfix or regression being targeted, not
18+
only the happy path.
19+
20+
## Test Structure
21+
22+
- Acceptance tests should exercise create → read → import → update → destroy
23+
where applicable. Import steps are particularly valuable because they
24+
validate that the read function can reconstruct state from the ID alone.
25+
- Use realistic fixture data; avoid asserting on transient or
26+
environment-specific fields without normalization.
27+
- Avoid hardcoded secrets or tokens in test files; use environment variables
28+
or test helpers.
29+
30+
## When Reviewing Test Changes
31+
32+
- If a test was deleted or weakened, explain why in the report and flag as
33+
at least MEDIUM unless the corresponding production code was also removed.
34+
- New skip conditions or `t.Skip` calls must include a clear justification.
35+
- Tests that depend on specific organization/repo names should use the
36+
shared test helpers/config, not hardcoded values.

0 commit comments

Comments
 (0)