Configure Copilot Code Review for the provider repo#3425
Configure Copilot Code Review for the provider repo#3425robert-crandall 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 |
There was a problem hiding this comment.
Pull request overview
Adds repository-wide and path-scoped Copilot Code Review (CCR) instruction files to align automated review feedback with this provider’s conventions (notably Go 1.24 toolchain assumptions and the intentional “no read-after-write” pattern from #2892), reducing low-value churn on PRs.
Changes:
- Added repo-wide CCR guidance in
.github/copilot-instructions.md(toolchain anchors, severity policy, provider-specific review priorities). - Added path-scoped CCR instruction files for provider Go code, tests, examples, and website docs under
.github/instructions/.
Show a summary per file
| File | Description |
|---|---|
| .github/copilot-instructions.md | Defines repo-wide CCR policy (Go/tooling context, severity rules, provider-specific review checklist). |
| .github/instructions/schema-and-state.instructions.md | Path-scoped guidance for github/**/*.go focusing on schema/state compatibility, CRUD behavior, and API safety. |
| .github/instructions/tests.instructions.md | Path-scoped guidance for github/**/*_test.go covering expectations for test updates and acceptance testing. |
| .github/instructions/examples.instructions.md | Path-scoped guidance for examples/** enforcing example structure, provider config rules, and sensitive handling. |
| .github/instructions/docs.instructions.md | Path-scoped guidance for website/** ensuring docs stay in sync with schema, imports, and permissions. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 5/5 changed files
- Comments generated: 0
deiga
left a comment
There was a problem hiding this comment.
This is looking really good!
- Correct Go version anchor: 1.24 -> 1.26 (matches go.mod). - Drop ValidateFunc everywhere; ValidateDiagFunc only (ValidateFunc is deprecated and not allowed in this repo). - Rewrite docs.instructions.md to target templates/** instead of the no-longer-existing website/**. Docs under docs/** are auto-generated; edits go in templates/, examples/, or resource Description fields. - Replace remaining website/ references in copilot-instructions.md with docs/ and templates/. - Add repository-rename convention to schema-and-state.instructions.md: attribute must be 'repository' (not ForceNew), plus computed 'repository_id', plus CustomizeDiff: diffRepository. - Add Delete 404 handling: treat as success, not error. - Require *Context CRUD variants and diag.Diagnostics on all new resources. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Thanks for the review @deiga! Addressed all 11 comments in 96bacb6:
Let me know if any of the framing on the repository-rename pattern needs tweaking; that one was new to me and I leaned on |
|
The new changes cover pretty much everything I noticed! Thanks @robert-crandall I just remembered that it could be useful to include mention that we now use
|
Add a 'terraform-plugin-testing Conventions' section to .github/instructions/tests.instructions.md covering: - Prefer ConfigStateChecks over the legacy Check / ComposeTestCheckFunc pattern. - Use ValueComparers (compare.ValuesSame / compare.ValuesDiffer) for cross-step value comparisons instead of custom pointer structs. - Don't flag legacy patterns in unmodified tests; only in new or substantially modified ones. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Good call! Added a |
|
@robert-crandall how about https://awesome-copilot.github.com/instructions/#file=instructions%2Fgo.instructions.md for the Go code? I have some customizations to suggest op top of that but it's a good starting point for generic Go code. |
Incorporates the upstream Go instructions from https://github.com/github/awesome-copilot/blob/main/instructions/go.instructions.md as a path-scoped instructions file (applyTo: **/*.go,**/go.mod,**/go.sum). A preamble subordinates the Go guidance to the repo's existing severity policy (HIGH/MEDIUM only, no LOW or nits) and notes the Go-version anchors in copilot-instructions.md plus the no-read-after-write exception, so CCR doesn't suddenly start surfacing style nits or fight provider-specific conventions. The main copilot-instructions.md now lists every path-scoped file so the set is discoverable. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Good idea! Done in 73cda9b |
This PR adds Copilot instructions to this repo.
Before the change?
.github/copilot-instructions.mdand no path-scoped.github/instructions/*.mdfiles, so Copilot Code Review (CCR) ran with no project-specific guidance. That meant a lot of churn on PRs: low-value nits, suggestions based on older Go toolchains, and missing context about provider-specific conventions (notably the intentional "no read-after-write" pattern, see [MAINT]: Remove read call from create/update #2892).After the change?
.github/copilot-instructions.mdwith the repo-wide review policy:range over int, or flag generics/min/max/clear/slices/mapsas unknown.Sensitive: trueon secret-bearing attributes, schemaDescription,ValidateFunc/ValidateDiagFuncon bounded inputs, examples for new resources, docs for schema changes, import ID format..github/instructions/:schema-and-state.instructions.md— applies togithub/**/*.go; covers schema/state compatibility, CRUD behavior, API safety.tests.instructions.md— applies togithub/**/*_test.go; coverage expectations and test structure.examples.instructions.md— applies toexamples/**; standard module layout, no nestedproviderblocks, sensitive variable handling.docs.instructions.md— applies towebsite/**; keeping docs in sync with schema, documenting import IDs and permission scopes.I think this should noticeably cut the contributor friction CCR has been generating, especially around Go-version false positives and stylistic nits. If something turns out to be too restrictive (or not restrictive enough), the instruction files are easy to iterate on.
Pull request checklist
Docs/tests checkboxes left unchecked: this PR only touches
.github/config for CCR. No provider code, no schema, no user-facing docs are affected.Does this introduce a breaking change?