diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 0000000000..457733c47e --- /dev/null +++ b/.github/copilot-instructions.md @@ -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. diff --git a/.github/instructions/docs.instructions.md b/.github/instructions/docs.instructions.md new file mode 100644 index 0000000000..3265087073 --- /dev/null +++ b/.github/instructions/docs.instructions.md @@ -0,0 +1,63 @@ +--- +applyTo: "templates/**" +--- + +# Docs and Templates Review + +Provider docs under `docs/**` are **auto-generated**. Do not edit files +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, + 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. + +## 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. + +## 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. + +## Style and Links + +- Internal links between docs pages should resolve. +- 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. diff --git a/.github/instructions/examples.instructions.md b/.github/instructions/examples.instructions.md new file mode 100644 index 0000000000..c0134e5bc2 --- /dev/null +++ b/.github/instructions/examples.instructions.md @@ -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 + +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. diff --git a/.github/instructions/go.instructions.md b/.github/instructions/go.instructions.md new file mode 100644 index 0000000000..42b1136b0f --- /dev/null +++ b/.github/instructions/go.instructions.md @@ -0,0 +1,406 @@ +--- +description: "Instructions for writing Go code following idiomatic Go practices and community standards" +applyTo: "**/*.go,**/go.mod,**/go.sum" +--- + +# Go Development Instructions + +> **How Copilot Code Review should use this file.** This document is the +> idiomatic-Go reference for any change touching `**/*.go`, `go.mod`, or +> `go.sum`. It applies on top of `.github/copilot-instructions.md` and the +> path-scoped files in this directory - it does **not** override them. +> +> In particular, the repo-wide **Severity and Nit Policy** still applies: +> +> - Only report `HIGH` and `MEDIUM` findings. Do not report `LOW` or nits. +> - Do not flag formatting, import ordering, identifier style, comment +> prose, or speculative refactors. Those belong to linters (`gofmt`, +> `goimports`, `go vet`, `golangci-lint`) and human reviewers, not CCR. +> - The Go-version anchors in `copilot-instructions.md` (Go 1.26, loop +> variable scoping, `range over int`, etc.) take precedence over any +> pre-1.22 patterns implied below. +> - The repo-specific exception in `schema-and-state.instructions.md` +> ("no read-after-write" in create/update) still holds even though +> generic Go style might suggest otherwise. +> +> Use the rest of this file as background for *what idiomatic Go looks +> like*. Only escalate a Go-style observation into a comment when it +> rises to a real correctness, concurrency, resource-leak, or security +> issue under the severity policy. + +Follow idiomatic Go practices and community standards when writing Go code. These instructions are based on [Effective Go](https://go.dev/doc/effective_go), [Go Code Review Comments](https://go.dev/wiki/CodeReviewComments), and [Google's Go Style Guide](https://google.github.io/styleguide/go/). + +## General Instructions + +- Write simple, clear, and idiomatic Go code +- Favor clarity and simplicity over cleverness +- Follow the principle of least surprise +- Keep the happy path left-aligned (minimize indentation) +- Return early to reduce nesting +- Prefer early return over if-else chains; use `if condition { return }` pattern to avoid else blocks +- Make the zero value useful +- Write self-documenting code with clear, descriptive names +- Document exported types, functions, methods, and packages +- Use Go modules for dependency management +- Leverage the Go standard library instead of reinventing the wheel (e.g., use `strings.Builder` for string concatenation, `filepath.Join` for path construction) +- Prefer standard library solutions over custom implementations when functionality exists +- Write comments in English by default; translate only upon user request +- Avoid using emoji in code and comments + +## Naming Conventions + +### Packages + +- Use lowercase, single-word package names +- Avoid underscores, hyphens, or mixedCaps +- Choose names that describe what the package provides, not what it contains +- Avoid generic names like `util`, `common`, or `base` +- Package names should be singular, not plural + +#### Package Declaration Rules (CRITICAL) +- **NEVER duplicate `package` declarations** - each Go file must have exactly ONE `package` line +- When editing an existing `.go` file: + - **PRESERVE** the existing `package` declaration - do not add another one + - If you need to replace the entire file content, start with the existing package name +- When creating a new `.go` file: + - **BEFORE writing any code**, check what package name other `.go` files in the same directory use + - Use the SAME package name as existing files in that directory + - If it's a new directory, use the directory name as the package name + - Write **exactly one** `package ` line at the very top of the file +- When using file creation or replacement tools: + - **ALWAYS verify** the target file doesn't already have a `package` declaration before adding one + - If replacing file content, include only ONE `package` declaration in the new content + - **NEVER** create files with multiple `package` lines or duplicate declarations + +### Variables and Functions + +- Use mixedCaps or MixedCaps (camelCase) rather than underscores +- Keep names short but descriptive +- Use single-letter variables only for very short scopes (like loop indices) +- Exported names start with a capital letter +- Unexported names start with a lowercase letter +- Avoid stuttering (e.g., avoid `http.HTTPServer`, prefer `http.Server`) + +### Interfaces + +- Name interfaces with -er suffix when possible (e.g., `Reader`, `Writer`, `Formatter`) +- Single-method interfaces should be named after the method (e.g., `Read` → `Reader`) +- Keep interfaces small and focused + +### Constants + +- Use MixedCaps for exported constants +- Use mixedCaps for unexported constants +- Group related constants using `const` blocks +- Consider using typed constants for better type safety + +## Code Style and Formatting + +### Formatting + +- Use `gofumpt` to quickly format code +- Use `goimports` to quickly manage imports automatically +- Use `golangci-lint fmt` to do final formatting pass +- Keep line length reasonable (no hard limit, but consider readability) +- Add blank lines to separate logical groups of code + +### Comments + +- Strive for self-documenting code; prefer clear variable names, function names, and code structure over comments +- Write comments only when necessary to explain complex logic, business rules, or non-obvious behavior +- Write comments in complete sentences in English by default +- Translate comments to other languages only upon specific user request +- Start sentences with the name of the thing being described +- Package comments should start with "Package [name]" +- Use line comments (`//`) for most comments +- Use block comments (`/* */`) sparingly, mainly for package documentation +- Document why, not what, unless the what is complex +- Avoid using emoji in comments and code + +### Error Handling + +- Check errors immediately after the function call +- Don't ignore errors using `_` unless you have a good reason (document why) +- Wrap errors with context using `fmt.Errorf` with `%w` verb +- Create custom error types when you need to check for specific errors +- Place error returns as the last return value +- Name error variables `err` +- Keep error messages lowercase and don't end with punctuation + +## Architecture and Project Structure + +### Package Organization + +- Follow standard Go project layout conventions +- Keep `main` packages in `cmd/` directory +- Put reusable packages in `pkg/` or `internal/` +- Use `internal/` for packages that shouldn't be imported by external projects +- Group related functionality into packages +- Avoid circular dependencies + +### Dependency Management + +- Use Go modules (`go.mod` and `go.sum`) +- Keep dependencies minimal +- Regularly update dependencies for security patches +- Use `go mod tidy` to clean up unused dependencies +- Vendor dependencies only when necessary + +## Type Safety and Language Features + +### Type Definitions + +- Define types to add meaning and type safety +- Use struct tags for JSON, XML, database mappings +- Prefer explicit type conversions +- Use type assertions carefully and check the second return value +- Prefer generics over unconstrained types; when an unconstrained type is truly needed, use the predeclared alias `any` instead of `interface{}` (Go 1.18+) + +### Pointers vs Values + +- Use pointer receivers for large structs or when you need to modify the receiver +- Use value receivers for small structs and when immutability is desired +- Use pointer parameters when you need to modify the argument or for large structs +- Use value parameters for small structs and when you want to prevent modification +- Be consistent within a type's method set +- Consider the zero value when choosing pointer vs value receivers + +### Interfaces and Composition + +- Accept interfaces, return concrete types +- Keep interfaces small (1-3 methods is ideal) +- Use embedding for composition +- Define interfaces close to where they're used, not where they're implemented +- Don't export interfaces unless necessary + +## Concurrency + +### Goroutines + +- Be cautious about creating goroutines in libraries; prefer letting the caller control concurrency +- If you must create goroutines in libraries, provide clear documentation and cleanup mechanisms +- Always know how a goroutine will exit +- Use `sync.WaitGroup` or channels to wait for goroutines +- Avoid goroutine leaks by ensuring cleanup + +### Channels + +- Use channels to communicate between goroutines +- Don't communicate by sharing memory; share memory by communicating +- Close channels from the sender side, not the receiver +- Use buffered channels when you know the capacity +- Use `select` for non-blocking operations + +### Synchronization + +- Use `sync.Mutex` for protecting shared state +- Keep critical sections small +- Use `sync.RWMutex` when you have many readers +- Choose between channels and mutexes based on the use case: use channels for communication, mutexes for protecting state +- Use `sync.Once` for one-time initialization +- WaitGroup usage by Go version: + - If `go >= 1.25` in `go.mod`, use the new `WaitGroup.Go` method ([documentation](https://pkg.go.dev/sync#WaitGroup)): + ```go + var wg sync.WaitGroup + wg.Go(task1) + wg.Go(task2) + wg.Wait() + ``` + - If `go < 1.25`, use the classic `Add`/`Done` pattern + +## Error Handling Patterns + +### Creating Errors + +- Use `errors.New` for simple static errors +- Use `fmt.Errorf` for dynamic errors +- Create custom error types for domain-specific errors +- Export error variables for sentinel errors +- Use `errors.Is` and `errors.As` for error checking + +### Error Propagation + +- Add context when propagating errors up the stack +- Don't log and return errors (choose one) +- Handle errors at the appropriate level +- Consider using structured errors for better debugging + +## Telemetry + +### Logging + +- Log messages should be sentences. + +## API Design + +### HTTP Handlers + +- Use `http.HandlerFunc` for simple handlers +- Implement `http.Handler` for handlers that need state +- Use middleware for cross-cutting concerns +- Set appropriate status codes and headers +- Handle errors gracefully and return appropriate error responses +- Router usage by Go version: + - If `go >= 1.22`, prefer the enhanced `net/http` `ServeMux` with pattern-based routing and method matching + - If `go < 1.22`, use the classic `ServeMux` and handle methods/paths manually (or use a third-party router when justified) + +### JSON APIs + +- Use struct tags to control JSON marshaling +- Validate input data +- Use pointers for optional fields +- Consider using `json.RawMessage` for delayed parsing +- Handle JSON errors appropriately + +### HTTP Clients + +- Keep the client struct focused on configuration and dependencies only (e.g., base URL, `*http.Client`, auth, default headers). It must not store any per-request state +- Do not store or cache `*http.Request` inside the client struct, and do not persist request-specific state across calls; instead, construct a fresh request per method invocation +- Methods should accept `context.Context` and input parameters, assemble the `*http.Request` locally (or via a short-lived builder/helper created per call), then call `c.httpClient.Do(req)` +- If request-building logic is reused, factor it into unexported helper functions or a per-call builder type; never keep `http.Request` (URL params, body, headers) as fields on the long-lived client +- Ensure the underlying `*http.Client` is configured (timeouts, transport) and is safe for concurrent use; avoid mutating `Transport` after first use +- Always set headers on the request instance you're sending, and close response bodies (`defer resp.Body.Close()`), handling errors appropriately + +## Performance Optimization + +### Memory Management + +- Minimize allocations in hot paths +- Reuse objects when possible (consider `sync.Pool`) +- Use value receivers for small structs +- Preallocate slices when size is known +- Avoid unnecessary string conversions + +### I/O: Readers and Buffers + +- Most `io.Reader` streams are consumable once; reading advances state. Do not assume a reader can be re-read without special handling +- If you must read data multiple times, buffer it once and recreate readers on demand: + - Use `io.ReadAll` (or a limited read) to obtain `[]byte`, then create fresh readers via `bytes.NewReader(buf)` or `bytes.NewBuffer(buf)` for each reuse + - For strings, use `strings.NewReader(s)`; you can `Seek(0, io.SeekStart)` on `*bytes.Reader` to rewind +- For HTTP requests, do not reuse a consumed `req.Body`. Instead: + - Keep the original payload as `[]byte` and set `req.Body = io.NopCloser(bytes.NewReader(buf))` before each send + - Prefer configuring `req.GetBody` so the transport can recreate the body for redirects/retries: `req.GetBody = func() (io.ReadCloser, error) { return io.NopCloser(bytes.NewReader(buf)), nil }` +- To duplicate a stream while reading, use `io.TeeReader` (copy to a buffer while passing through) or write to multiple sinks with `io.MultiWriter` +- Reusing buffered readers: call `(*bufio.Reader).Reset(r)` to attach to a new underlying reader; do not expect it to "rewind" unless the source supports seeking +- For large payloads, avoid unbounded buffering; consider streaming, `io.LimitReader`, or on-disk temporary storage to control memory + +- Use `io.Pipe` to stream without buffering the whole payload: + - Write to `*io.PipeWriter` in a separate goroutine while the reader consumes + - Always close the writer; use `CloseWithError(err)` on failures + - `io.Pipe` is for streaming, not rewinding or making readers reusable + +- **Warning:** When using `io.Pipe` (especially with multipart writers), all writes must be performed in strict, sequential order. Do not write concurrently or out of order - multipart boundaries and chunk order must be preserved. Out-of-order or parallel writes can corrupt the stream and result in errors. + +- Streaming multipart/form-data with `io.Pipe`: + - `pr, pw := io.Pipe()`; `mw := multipart.NewWriter(pw)`; use `pr` as the HTTP request body + - Set `Content-Type` to `mw.FormDataContentType()` + - In a goroutine: write all parts to `mw` in the correct order; on error `pw.CloseWithError(err)`; on success `mw.Close()` then `pw.Close()` + - Do not store request/in-flight form state on a long-lived client; build per call + - Streamed bodies are not rewindable; for retries/redirects, buffer small payloads or provide `GetBody` + +### Profiling + +- Use built-in profiling tools (`pprof`) +- Benchmark critical code paths +- Profile before optimizing +- Focus on algorithmic improvements first +- Consider using `testing.B` for benchmarks + +## Testing + +### Test Organization + +- Keep tests in the same package (white-box testing) +- Use `_test` package suffix for black-box testing +- Name test files with `_test.go` suffix +- Place test files next to the code they test + +### Writing Tests + +- Name tests descriptively + - Use `TestFunctionName_scenario` for exported functions + - Use `Test_functionName_scenario` for un-exported functions + - Use `_` to separate logical parts of the test name +- Use table-driven tests for multiple test cases +- Use subtests with `t.Run` for better organization +- Test both success and error cases +- Consider using `google/go-cmp` if needed, but don't over-complicate simple tests + +### Test Helpers + +- Mark helper functions with `t.Helper()` +- Create test fixtures for complex setup +- Use `testing.TB` interface for functions used in tests and benchmarks +- Clean up resources using `t.Cleanup()` + +## Security Best Practices + +### Input Validation + +- Validate all external input +- Use strong typing to prevent invalid states +- Sanitize data before using in SQL queries +- Be careful with file paths from user input +- Validate and escape data for different contexts (HTML, SQL, shell) + +### Cryptography + +- Use standard library crypto packages +- Don't implement your own cryptography +- Use crypto/rand for random number generation +- Store passwords using bcrypt, scrypt, or argon2 (consider golang.org/x/crypto for additional options) +- Use TLS for network communication + +## Documentation + +### Code Documentation + +- Prioritize self-documenting code through clear naming and structure +- Document all exported symbols with clear, concise explanations +- Start documentation with the symbol name +- Write documentation in English by default +- Use examples in documentation when helpful +- Keep documentation close to code +- Update documentation when code changes +- Avoid emoji in documentation and comments + +### README and Documentation Files + +- Include clear setup instructions +- Document dependencies and requirements +- Provide usage examples +- Document configuration options +- Include troubleshooting section + +## Tools and Development Workflow + +### Essential Tools + +- `go fmt`: Format code +- `go vet`: Find suspicious constructs +- `golangci-lint`: Additional linting (golint is deprecated) +- `go test`: Run tests +- `go mod`: Manage dependencies +- `go generate`: Code generation + +### Development Practices + +- Run tests before committing +- Use pre-commit hooks for formatting and linting +- Keep commits focused and atomic +- Write meaningful commit messages +- Review diffs before committing + +## Common Pitfalls to Avoid + +- Not checking errors +- Ignoring race conditions +- Creating goroutine leaks +- Not using defer for cleanup +- Modifying maps concurrently +- Not understanding nil interfaces vs nil pointers +- Forgetting to close resources (files, connections) +- Using global variables unnecessarily +- Overusing unconstrained types (e.g., `any`); prefer specific types or generic type parameters with constraints. If an unconstrained type is required, use `any` rather than `interface{}` +- Not considering the zero value of types +- **Creating duplicate `package` declarations** - this is a compile error; always check existing files before adding package declarations diff --git a/.github/instructions/schema-and-state.instructions.md b/.github/instructions/schema-and-state.instructions.md new file mode 100644 index 0000000000..8209501d8e --- /dev/null +++ b/.github/instructions/schema-and-state.instructions.md @@ -0,0 +1,105 @@ +--- +applyTo: "github/**/*.go" +--- + +# Provider Source Review (Schema, State, API) + +These rules apply to all provider Go source files under `github/`. Combine +with the repo-wide checklist in `.github/copilot-instructions.md`. + +## Schema Changes Are User-Visible + +Any change to `schema.Schema` (`Type`, `Optional`, `Required`, `Computed`, +`ForceNew`, `Default`, `Sensitive`, `Description`) is potentially breaking. +Flag all schema diffs and verify: + +- Attribute removals or renames have a deprecation cycle or `moved`/state + migration guidance. +- `Optional` → `Required` transitions are called out as breaking. +- New `ForceNew` flags on existing attributes are called out as breaking + (forces resource replacement). +- New attributes are `Optional` with a `Default` where reasonable to avoid + forcing existing users to update their configs. +- All attributes carry a `Description` string (used for docs generation). +- Attributes accepting bounded values declare `ValidateDiagFunc` so + invalid input fails at plan time, not apply time. `ValidateFunc` is + deprecated and not allowed in this repo - do not suggest it. +- Attributes holding tokens, secrets, or private keys are marked + `Sensitive: true`. + +## Repository as a Required Argument + +When a resource accepts a repository name as a required argument, follow +the provider's rename-safe convention so users can rename a repository +without triggering a destroy/recreate cycle: + +- Name the attribute `repository` (not `repo`, not `repository_name`). +- Do **not** mark `repository` as `ForceNew`, even when the underlying + resource needs to be recreated on most changes. The rename handling + below decides when replacement is actually required. +- Add a `Computed` attribute called `repository_id` that holds the + GitHub repository's numeric ID. +- Set `CustomizeDiff: diffRepository` on the resource (or include it via + `customdiff.All(...)` when multiple `CustomizeDiff` funcs are needed). + This compares the stored `repository_id` against the current ID for + the named repository and only forces replacement when the underlying + repository actually changed, not when it was merely renamed. + +Flag any new resource that takes a repository as required input but is +missing this pattern. + +## State and Drift + +- Read functions must populate every state attribute from API responses so + `terraform import` works from the resource ID alone. +- Verify the read path does not produce values that differ from what create/ + update wrote (causes perpetual diffs). +- Watch list/set ordering: prefer `schema.TypeSet` or stable sort when the + GitHub API does not return deterministic order. +- Empty vs. null handling must be intentional and consistent between create, + read, and update. +- Diff suppression (`DiffSuppressFunc`) and normalization should be reviewed + for correctness whenever schema is touched. + +## CRUD Behavior + +- All CRUD functions (`Create`, `Read`, `Update`, `Delete`, and the + importer) must use their `*Context` variants + (`CreateContext`/`ReadContext`/`UpdateContext`/`DeleteContext` and + `StateContext` on importers) and return `diag.Diagnostics`. Flag any + new resource that uses the deprecated non-context variants. +- Update paths must not accidentally force resource replacement or wipe + optional fields that the user did not change. +- Nil-check API response fields before dereferencing. +- Classify errors: + - In **Read**, a 404 from GitHub means "remove from state" - call + `d.SetId("")` and return nil. Other errors should bubble up. + - In **Delete**, a 404 from GitHub means the object is already gone - + treat it as success and return nil, not as an error. + - Other unexpected status codes should bubble up. +- Respect `context.Context` cancellation and any configured timeouts. + +## Repository-Specific: No Read-After-Write + +**Do not flag** create or update functions that return `nil` instead of +calling the resource's read function. This is intentional in this provider to +minimize API calls against GitHub rate limits and to avoid stale reads from +eventually-consistent endpoints. See +[#2892](https://github.com/integrations/terraform-provider-github/issues/2892). + +## API Safety and Performance + +- Flag new N+1 access patterns over GitHub APIs. +- Verify pagination is handled (`ListOptions` / `NextPage` loops) on any + endpoint that returns a list. +- Check for rate-limit-sensitive loops; consider caching or batching where + appropriate. +- Sensitive values must never appear in log output, even at debug/trace + level. + +## Security + +- Token, credential, and webhook secret handling must follow least + privilege. +- New API calls should document the GitHub permission scope they require. +- Do not hardcode secrets anywhere in source. diff --git a/.github/instructions/tests.instructions.md b/.github/instructions/tests.instructions.md new file mode 100644 index 0000000000..61b2ede913 --- /dev/null +++ b/.github/instructions/tests.instructions.md @@ -0,0 +1,58 @@ +--- +applyTo: "github/**/*_test.go" +--- + +# Provider Test Review + +These rules apply to test files under `github/`. Combine with the repo-wide +checklist in `.github/copilot-instructions.md`. + +## Coverage Expectations + +- When behavior in a resource or data source changes, there must be a + matching test change. Flag PRs where production code changed but tests + did not. +- Prefer acceptance tests (`TF_ACC=1`) for any API-facing change. Unit-only + coverage is rarely sufficient for schema or CRUD changes. +- Tests should assert the specific bugfix or regression being targeted, not + only the happy path. + +## Test Structure + +- Acceptance tests should exercise create → read → import → update → destroy + where applicable. Import steps are particularly valuable because they + validate that the read function can reconstruct state from the ID alone. +- Use realistic fixture data; avoid asserting on transient or + environment-specific fields without normalization. +- Avoid hardcoded secrets or tokens in test files; use environment variables + or test helpers. + +## terraform-plugin-testing Conventions + +Tests in this repo use +[`github.com/hashicorp/terraform-plugin-testing`](https://pkg.go.dev/github.com/hashicorp/terraform-plugin-testing) +and follow its modern conventions. Hold new and changed tests to these +patterns: + +- Prefer `ConfigStateChecks` (state-check functions from the + `statecheck` package) over the older `Check` / + `resource.ComposeTestCheckFunc` pattern. Flag new `TestStep`s that + use `Check` with the legacy `resource.TestCheckResourceAttr*` helpers + when an equivalent `statecheck` exists. +- For comparing values across steps (e.g. to assert a value did or did + not change between Apply runs), use + [`ValueComparers`](https://developer.hashicorp.com/terraform/plugin/testing/acceptance-tests/value-comparers) + via `compare.ValuesSame` / `compare.ValuesDiffer` instead of the older + pattern of stashing pointers to attribute values in custom structs and + comparing them by hand. +- Existing tests using the legacy `Check` pattern do not need to be + rewritten as part of an unrelated change. Only flag the legacy pattern + in new tests, or in tests that are being substantially modified. + +## When Reviewing Test Changes + +- If a test was deleted or weakened, explain why in the report and flag as + at least MEDIUM unless the corresponding production code was also removed. +- New skip conditions or `t.Skip` calls must include a clear justification. +- Tests that depend on specific organization/repo names should use the + shared test helpers/config, not hardcoded values.