From bae3fab95b2b58b4d70d9b52e8988d80b92b51ce Mon Sep 17 00:00:00 2001 From: Timo Sand Date: Sun, 15 Feb 2026 23:28:53 +0200 Subject: [PATCH 1/7] Proposal for `ARCHITECTURE.md` and `MAINTERS.md` content Signed-off-by: Timo Sand --- ARCHITECTURE.md | 619 ++++++++++++++++++++++++++++++++++++++++++++++++ MAINTAINERS.md | 19 ++ 2 files changed, 638 insertions(+) create mode 100644 ARCHITECTURE.md create mode 100644 MAINTAINERS.md diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md new file mode 100644 index 0000000000..d06d0a9230 --- /dev/null +++ b/ARCHITECTURE.md @@ -0,0 +1,619 @@ +# Architecture Guide + +This document serves as a guide for contributors implementing new features and resources in the Terraform Provider for GitHub. + +- [Architecture Guide](#architecture-guide) + - [Module Map](#module-map) + - [Core Principles](#core-principles) + - [1. One Resource = One API Entity](#1-one-resource--one-api-entity) + - [2. Minimize API Calls](#2-minimize-api-calls) + - [3. API-Only Operations](#3-api-only-operations) + - [Resource Design](#resource-design) + - [File Organization](#file-organization) + - [Resource Structure](#resource-structure) + - [Schema Field Guidelines](#schema-field-guidelines) + - [ID Patterns](#id-patterns) + - [Implementation Patterns](#implementation-patterns) + - [CRUD Function Signatures](#crud-function-signatures) + - [Accessing the API Client](#accessing-the-api-client) + - [Error Handling](#error-handling) + - [State Migrations](#state-migrations) + - [Logging](#logging) + - [Testing](#testing) + - [Test Structure](#test-structure) + - [Test Modes](#test-modes) + - [Running Tests](#running-tests) + - [Debugging Tests](#debugging-tests) + - [Gotchas \& Known Issues](#gotchas--known-issues) + - [API Preview Headers](#api-preview-headers) + - [Deprecated Resources](#deprecated-resources) + - [Known Limitations](#known-limitations) + - [Workarounds in Code](#workarounds-in-code) + - [Pending go-github Updates](#pending-go-github-updates) + - [Appendix](#appendix) + - [Common Utilities](#common-utilities) + - [Naming Conventions](#naming-conventions) + - [Decision Log](#decision-log) + - [January 2026](#january-2026) + - [Use StateUpgraders for State Migrations](#use-stateupgraders-for-state-migrations) + - [Explicit Authentication Configuration](#explicit-authentication-configuration) + - [Transport Layer Rework](#transport-layer-rework) + - [No Local Git CLI Support](#no-local-git-cli-support) + - [2025](#2025) + - [Replace `log` Package with `tflog`](#replace-log-package-with-tflog) + - [Finalize SDK v2 Migration](#finalize-sdk-v2-migration) + +--- + +## Module Map + +```text +terraform-provider-github/ +├── github/ +│ ├── provider.go # Entry point, registers all resources/data sources +│ ├── config.go # Auth setup, HTTP client, rate limiting, transport +│ │ +│ ├── resource_github_*.go # Resource implementations +│ ├── resource_*_migration.go # Resource state migration functions (StateUpgraders) +│ ├── data_source_github_*.go # Data source implementations +│ │ +│ │ +│ ├── util.go # Core utilities (ID parsing, validation) +│ ├── util_*.go # Domain utilities (rules, labels, etc.) +│ │ +│ └── transport.go # Custom HTTP transport with ETag caching +│ +├── ARCHITECTURE.md # This file - implementation guide +├── MAINTAINERS.md # Maintainers, decision log, contributors +└── CONTRIBUTING.md # How to contribute +``` + +--- + +## Core Principles + +### 1. One Resource = One API Entity + +Each Terraform resource should map to a single GitHub API entity. Avoid creating resources that combine multiple API concerns. + +**Do:** + +```go +// Manages a single repository +func resourceGithubRepository() *schema.Resource { ... } + +// Manages repository topics separately +func resourceGithubRepositoryTopics() *schema.Resource { ... } +``` + +**Don't:** + +```go +// Don't combine unrelated API entities into one resource +func resourceGithubRepositoryWithTopicsAndLabels() *schema.Resource { ... } +``` + +### 2. Minimize API Calls + +Each resource should use the minimum number of API calls necessary. Consolidate operations where possible and avoid redundant reads. + +**Do:** + +```go +func resourceExampleRead(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { + // Single API call to get all needed data + resource, _, err := client.Resources.Get(ctx, owner, name) + if err != nil { + return diag.FromErr(err) + } + // Set all attributes from single response + d.Set("field1", resource.Field1) + d.Set("field2", resource.Field2) + return nil +} +``` + +**Don't:** + +```go +func resourceExampleRead(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { + // Don't make separate calls for each field + field1, _ := client.Resources.GetField1(ctx, owner, name) + field2, _ := client.Resources.GetField2(ctx, owner, name) + // ... +} +``` + +### 3. API-Only Operations + +We do not support using local git CLI to operate on repositories. All operations must go through the GitHub API. + +**Do:** + +```go +// Use go-github client for all git operations +_, _, err := client.Git.CreateRef(ctx, owner, repo, ref) +``` + +**Don't:** + +```go +// Never shell out to git CLI +exec.Command("git", "push", "origin", "main") +``` + +--- + +## Resource Design + +### File Organization + +Resources follow a consistent file naming and organization pattern: + +```text +github/ +├── resource_github_.go # Main resource implementation +├── resource_github__test.go # Acceptance tests +├── resource_github__migration.go # State migration functions (if needed) +├── data_source_github_.go # Data source implementation +├── data_source_github__test.go # Data source tests +└── util_.go # Domain-specific utilities +``` + +### Resource Structure + +Use context-aware CRUD functions with the `*Context` suffix: + +```go +func resourceGithubExample() *schema.Resource { + return &schema.Resource{ + CreateContext: resourceGithubExampleCreate, + ReadContext: resourceGithubExampleRead, + UpdateContext: resourceGithubExampleUpdate, + DeleteContext: resourceGithubExampleDelete, + Importer: &schema.ResourceImporter{ + StateContext: resourceGithubExampleImport, + }, + + // Include SchemaVersion and StateUpgraders if state migrations exist + SchemaVersion: 1, + StateUpgraders: []schema.StateUpgrader{ + { + Type: resourceGithubExampleResourceV0().CoreConfigSchema().ImpliedType(), + Upgrade: resourceGithubExampleInstanceStateUpgradeV0, + Version: 0, + }, + }, + + Schema: map[string]*schema.Schema{ + // Schema definition + }, + } +} +``` + +### Schema Field Guidelines + +Full reference: [Schema Behaviors](https://developer.hashicorp.com/terraform/plugin/sdkv2/schemas/schema-behaviors) + +**Primitive options:** + +- `Type` — field type (`TypeString`, `TypeBool`, `TypeInt`, `TypeFloat`, `TypeList`, `TypeSet`, `TypeMap`) +- `Description` — human-readable description (always include) +- `Elem` — element type for `TypeList`, `TypeSet`, and `TypeMap` fields (e.g., `&schema.Schema{Type: schema.TypeString}` or a nested `&schema.Resource{}`) +- `Default` — static default value when field is not set in config +- `DefaultFunc` — dynamic default (e.g., read from env var via `schema.EnvDefaultFunc`) + +**Behavior flags:** + +- `Required` — must be provided in config (mutually exclusive with `Optional`, `Computed`) +- `Optional` — may be omitted from config +- `Computed` — set by the provider (API-derived); combine with `Optional` for optional fields with server defaults +- `ForceNew` — changing this field destroys and recreates the resource +- `Sensitive` — value is masked in plan/state output (secrets, tokens) + +**Validation:** + +- `ValidateDiagFunc` — validate field value with diagnostics (preferred) + +**Constraints:** + +- `MaxItems` / `MinItems` — cardinality bounds for `TypeList` and `TypeSet` +- `ConflictsWith` — list of field paths that cannot be set together with this field +- `ExactlyOneOf` — exactly one of these fields must be set +- `AtLeastOneOf` — at least one of these fields must be set +- `RequiredWith` — these fields must all be set if this field is set + +**Advanced:** + +- `StateFunc` — transform value before storing in state (e.g., normalize to lowercase) +- `DiffSuppressFunc` — suppress plan diffs when old and new values are semantically equal +- `DiffSuppressOnRefresh` — also apply `DiffSuppressFunc` during refresh +- `Set` — custom hash function for `TypeSet` elements +- `Deprecated` — marks field as deprecated with a message shown to users + +### ID Patterns + +For single-part IDs (most common): + +```go +d.SetId(resource.GetName()) +``` + +For composite IDs, use `buildID` to create and `parseID2`/`parseID3`/`parseID4` to parse: + +```go +// Two-part ID +id, err := buildID(owner, name) +d.SetId(id) +// Parse: +owner, name, err := parseID2(id) + +// Three-part ID +id, err := buildID(owner, repo, name) +d.SetId(id) +// Parse: +owner, repo, name, err := parseID3(id) + +// Four-part ID +owner, repo, env, name, err := parseID4(id) + +// IDs with special characters (colons, etc.) +id, err := buildID(escapeIDPart(part1), part2) +``` + +> **Note:** The legacy functions `buildTwoPartID`, `parseTwoPartID`, `buildThreePartID`, and `parseThreePartID` are deprecated. Use `buildID` and `parseID2`/`parseID3`/`parseID4` instead. + +--- + +## Implementation Patterns + +### CRUD Function Signatures + +```go +func resourceGithubExampleCreate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { + client := meta.(*Owner).v3client + owner := meta.(*Owner).name + + // Implementation + return nil // Never call Read at end of Create, set any Computed fields in Create +} + +func resourceGithubExampleRead(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { + // Implementation + return nil +} + +func resourceGithubExampleUpdate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { + // Implementation + return nil // Never call Read at end of Update, set any Computed fields in Update +} + +func resourceGithubExampleDelete(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { + // Implementation + return nil // Never call Read after Delete +} +``` + +### Accessing the API Client + +```go +func resourceExampleRead(ctx context.Context, d *schema.ResourceData, m any) diag.Diagnostics { + meta := m.(*Owner) + // REST API client (go-github v82) + client := meta.v3client + + // GraphQL client (for queries not available in REST) + v4client := meta.v4client + + // Owner context + owner := meta.name + isOrg := meta.IsOrganization + orgID := meta.id + + // ... +} +``` + +### Error Handling + +Handle 404s gracefully by removing from state: + +```go +resource, _, err := client.Resources.Get(ctx, owner, name) +if err != nil { + var ghErr *github.ErrorResponse + if errors.As(err, &ghErr) { + if ghErr.Response.StatusCode == http.StatusNotFound { + log.Printf("[INFO] Removing %s from state because it no longer exists", name) + d.SetId("") + return nil + } + } + return diag.FromErr(err) +} +``` + +Or use the helper function: + +```go +if err := deleteResourceOn404AndSwallow304OtherwiseReturnError(err, d, "resource %s", name); err != nil { + return diag.FromErr(err) +} +``` + +### State Migrations + +When adding new fields or changing schema, use `StateUpgraders` (not the deprecated `MigrateState`): + +**Migration file (`resource_github_example_migration.go`):** + +```go +// resourceGithubExampleResourceV0 returns the schema for version 0 +func resourceGithubExampleResourceV0() *schema.Resource { + return &schema.Resource{ + Schema: map[string]*schema.Schema{ + // Previous schema version + }, + } +} + +// resourceGithubExampleInstanceStateUpgradeV0 migrates from version 0 to 1 +func resourceGithubExampleInstanceStateUpgradeV0(ctx context.Context, rawState map[string]any, meta any) (map[string]any, error) { + log.Printf("[DEBUG] State before migration: %#v", rawState) + + // Add new field with default value + if _, ok := rawState["new_field"]; !ok { + rawState["new_field"] = "default_value" + } + + log.Printf("[DEBUG] State after migration: %#v", rawState) + return rawState, nil +} +``` + +**Register in resource:** + +```go +SchemaVersion: 1, +StateUpgraders: []schema.StateUpgrader{ + { + Type: resourceGithubExampleResourceV0().CoreConfigSchema().ImpliedType(), + Upgrade: resourceGithubExampleInstanceStateUpgradeV0, + Version: 0, + }, +}, +``` + +### Logging + +Use `tflog` for structured logging (replacing `log` package): + +```go +import "github.com/hashicorp/terraform-plugin-log/tflog" + +func resourceExampleCreate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { + tflog.Debug(ctx, "Creating resource", map[string]any{ + "name": name, + "owner": owner, + }) + // ... +} +``` + +**Note:** Migration from `log` to `tflog` is in progress. New code should use `tflog`. + +--- + +## Testing + +### Test Structure + +```go +func TestAccGithubExample(t *testing.T) { + + t.Run("creates resource without error", func(t *testing.T) { + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) + testResourceName := fmt.Sprintf("%s%s", testResourcePrefix, randomID) + config := fmt.Sprintf(` + resource "github_example" "test" { + name = "%s" + } + `, testResourceName) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnauthenticated(t) }, + ProviderFactories: providerFactories, + Steps: []resource.TestStep{ + { + Config: config, + Check: resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( "github_example.test", "name", testResourceName ), + ), + }, + }, + }) + }) +} +``` + +### Test Modes + +Use `skipUnauthenticated(t)`, `skipUnlessHasOrgs(t)`, `skipUnlessHasPaidOrgs(t)`, `skipUnlessEnterprise(t)`, `skipUnlessMode(t, testModes...)` functions to run tests in appropriate contexts: + +| Mode | Environment Variables Required | +| -------------- | -------------------------------------- | +| `anonymous` | None (read-only, GitHub.com only) | +| `individual` | `GITHUB_TOKEN` + `GITHUB_OWNER` | +| `organization` | `GITHUB_TOKEN` + `GITHUB_ORGANIZATION` | +| `enterprise` | Enterprise deployment configuration | + +### Running Tests + +```bash +# Run specific test +make testacc T=TestAccGithubExample +``` + +### Debugging Tests + +```bash +# With debug logging +TF_LOG=DEBUG make testacc T=TestAccGithubExample +``` + +--- + +## Gotchas & Known Issues + +This section documents provider-specific quirks and known limitations discovered through development. + +### API Preview Headers + +- Stone Crop preview header is still required for some GraphQL features (`config.go:64-65`) +- The `previewHeaderInjectorTransport` handles automatic header injection for these cases + +### Deprecated Resources + +The following resources are deprecated and will be removed in future versions: + +| Deprecated Resource | Replacement | +| -------------------------------------------- | ------------------------------------------------- | +| `github_organization_security_manager` | `github_organization_role_team` | +| `github_organization_custom_role` | `github_organization_repository_role` | +| `github_repository_deployment_branch_policy` | `github_repository_environment_deployment_policy` | +| `github_organization_project` | None (Classic Projects API removed) | +| `github_project_card` | None (Classic Projects API removed) | +| `github_project_column` | None (Classic Projects API removed) | +| `github_repository_project` | None (Classic Projects API removed) | + +### Known Limitations + +- **Branch Protection `contexts`**: Deprecated, use the `checks` array instead +- **Runner Groups**: Selected repository IDs are not exposed via API (`resource_github_actions_runner_group.go:179`) +- **Organization Settings**: Test requires manual cleanup (`resource_github_organization_settings_test.go:11`) +- **Repository Search**: Tests may hit rate limits (`data_source_github_repositories_test.go:12`) + +### Workarounds in Code + +- **EMU with SSO**: Odd behavior with user tokens when using Enterprise Managed Users (`resource_github_enterprise_organization.go:122`) + +### Pending go-github Updates + +Several features are blocked waiting for go-github v68+: + +- `data_source_github_organization_repository_role.go:56` +- `resource_github_organization_repository_role.go:102` +- `data_source_github_organization_role_users.go:41` +- `data_source_github_organization_role_teams.go:51` + +--- + +## Appendix + +### Common Utilities + +| Function | Purpose | +| ----------------------------------------------------------- | ---------------------------------------------- | +| `buildID(parts...)` | Create composite ID (e.g., `"a:b"`, `"a:b:c"`) | +| `parseID2(id)` | Parse two-part composite ID | +| `parseID3(id)` | Parse three-part composite ID | +| `parseID4(id)` | Parse four-part composite ID | +| `escapeIDPart(part)` | Escape colons in ID parts | +| `wrapErrors([]error)` | Convert errors to diagnostics | +| `checkOrganization(meta)` | Verify org context | +| `getTeamID(idOrSlug, meta)` | Resolve team ID from ID or slug | +| `getTeamSlug(idOrSlug, meta)` | Resolve team slug from ID or slug | +| `expandStringList([]any)` | Convert to `[]string` | +| `flattenStringList([]string)` | Convert to `[]any` | +| `deleteResourceOn404AndSwallow304OtherwiseReturnError(...)` | Handle 404/304 responses | + +### Naming Conventions + +| Component | Pattern | Example | +| -------------------- | ------------------------------------------------ | ------------------------------------------------ | +| Resource function | `resourceGithub` | `resourceGithubRepository` | +| Data source function | `dataSourceGithub` | `dataSourceGithubRepository` | +| CRUD functions | `resourceGithub` | `resourceGithubRepositoryCreate` | +| Migration function | `resourceGithubInstanceStateUpgradeV` | `resourceGithubRepositoryInstanceStateUpgradeV0` | +| Schema function | `resourceGithubResourceV` | `resourceGithubRepositoryResourceV0` | +| Test function | `TestAccGithub` | `TestAccGithubRepository` | +| Utility file | `util_.go` | `util_rules.go` | + +--- + +## Decision Log + +### January 2026 + +#### Use StateUpgraders for State Migrations + +**Decision:** Use `StateUpgraders` instead of the deprecated `MigrateState` function. + +**Rationale:** `StateUpgraders` provides a cleaner, more maintainable approach to state migrations that works better with the SDK v2 architecture. + +**Implementation:** + +- Create `resource_github__migration.go` with versioned schema and upgrade functions +- Register in resource with `SchemaVersion` and `StateUpgraders` +- See [ARCHITECTURE.md](ARCHITECTURE.md#state-migrations) for implementation pattern + +#### Explicit Authentication Configuration + +**Decision:** Make all authentication concerns of the provider entirely explicit. Users must explicitly configure their authentication method. + +**Rationale:** Implicit auth detection can lead to confusion and security issues. Explicit configuration makes the provider's behavior predictable and auditable. + +**Reference:** + +#### Transport Layer Rework + +**Decision:** Rework the transport layer to utilize: + +- [`github-conditional-http-transport`](https://github.com/bored-engineer/github-conditional-http-transport) for conditional requests +- [`go-github-ratelimit`](https://github.com/gofri/go-github-ratelimit) for rate limiting + +**Rationale:** These libraries provide better handling of GitHub API rate limits and conditional requests than our current custom implementation. + +**Reference:** + +#### No Local Git CLI Support + +**Decision:** Do not support using local git CLI to operate on repositories; use purely API operations. + +**Rationale:** API-only operations ensure consistency, security, and avoid environment dependencies. The provider should not assume git is installed or configured on the user's machine. + +### 2025 + +#### Replace `log` Package with `tflog` + +**Decision:** Replace all usage of the standard `log` package with `tflog` from terraform-plugin-log. + +**Rationale:** `tflog` provides structured logging that integrates better with Terraform's logging infrastructure and supports log filtering, structured fields, and proper log levels. + +**Migration pattern:** + +```go +// Before +log.Printf("[DEBUG] Creating resource: %s", name) + +// After +tflog.Debug(ctx, "Creating resource", map[string]any{"name": name}) +``` + +#### Finalize SDK v2 Migration + +**Decision:** Complete migration to Terraform Plugin SDK v2. + +**Rationale:** SDK v2 provides better diagnostics, context-aware functions, and improved schema validation. + +**Key changes:** + +- Use `*Context` functions (`CreateContext`, `ReadContext`, etc.) +- Use `ValidateDiagFunc` instead of `ValidateFunc` +- Use `diag.Diagnostics` for error returns +- Use `any` instead of `interface{}` + +**Reference:** + +--- diff --git a/MAINTAINERS.md b/MAINTAINERS.md new file mode 100644 index 0000000000..60e2bae444 --- /dev/null +++ b/MAINTAINERS.md @@ -0,0 +1,19 @@ +# Maintainers + +This document lists the current maintainers of the Terraform Provider for GitHub. + +## Current Maintainers + +| Name | GitHub | +| ------------------ | ---------------------------------------------------------- | +| Diógenes Fernandes | [@diofeher](https://github.com/diofeher) | +| Nick Floyd | [@nickfloyd](https://github.com/nickfloyd) | +| Steve Hipwell | [@stevehipwell](https://github.com/stevehipwell) | +| Viacheslav Kudinov | [@ViacheslavKudinov](https://github.com/ViacheslavKudinov) | +| Timo Sand | [@deiga](https://github.com/deiga) | + +## Emeritus Maintainers (in alphabetical order) + +| Name | GitHub | +| --------------- | -------------------------------------------- | +| Keegan Campbell | [@kfcampbell](https://github.com/kfcampbell) | From 7afb7dbf0d741e86c1feac5dff7b2dcdad2e0e80 Mon Sep 17 00:00:00 2001 From: Timo Sand Date: Sun, 15 Feb 2026 23:29:56 +0200 Subject: [PATCH 2/7] Mark `parseTwoPartID` and `parseThreePartID` as deprecated Signed-off-by: Timo Sand --- github/util.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/github/util.go b/github/util.go index 0d9bc0d32c..ac22fd884c 100644 --- a/github/util.go +++ b/github/util.go @@ -142,6 +142,7 @@ func validateValueFunc(values []string) schema.SchemaValidateDiagFunc { } // return the pieces of id `left:right` as left, right. +// @deprecated: use parseID2 instead. func parseTwoPartID(id, left, right string) (string, string, error) { parts := strings.SplitN(id, ":", 2) if len(parts) != 2 { @@ -157,6 +158,7 @@ func buildTwoPartID(a, b string) string { } // return the pieces of id `left:center:right` as left, center, right. +// @deprecated: use parseID3 instead. func parseThreePartID(id, left, center, right string) (string, string, string, error) { parts := strings.SplitN(id, ":", 3) if len(parts) != 3 { From cc5b69e96cf46d08a5de502f16dc576fde0beec5 Mon Sep 17 00:00:00 2001 From: Timo Sand Date: Sun, 15 Feb 2026 23:33:25 +0200 Subject: [PATCH 3/7] Update `CONTRIBUTING.md` Signed-off-by: Timo Sand --- CONTRIBUTING.md | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a924c1be9f..0116ebddf8 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -13,8 +13,10 @@ Before submitting an issue or a pull request, please search the repository for e 1. Fork and clone the repository. 2. Create a new branch: `git switch -c my-branch-name`. 3. Make your change, add tests, and make sure the tests still pass. -4. Push to your fork and submit a pull request. -5. Pat yourself on the back and wait for your pull request to be reviewed and merged. +4. Update or add documentation to reflect your changes. +5. Ensure formatting and linting are passing. +6. Push to your fork and submit a pull request. +7. Pat yourself on the back and wait for your pull request to be reviewed and merged. Here are a few things you can do that will increase the likelihood of your pull request being accepted: @@ -47,7 +49,7 @@ Once you have the repository cloned, there's a couple of additional steps you'll ``` - Build the project with `make build` -- Try an example test run from the default (`main`) branch, like `TF_LOG=DEBUG TF_ACC=1 go test -v ./... -run ^TestAccGithubRepositories`. All those tests should pass. +- Try an example test run from the default (`main`) branch, like `TF_LOG=DEBUG make testacc T=TestAccGithubRepositories`. All those tests should pass. ### Local Development Iteration @@ -55,13 +57,13 @@ Once you have the repository cloned, there's a couple of additional steps you'll 2. Run your test and observe it fail. Enabling debug output allows for observing the underlying requests and responses made as well as viewing state (search `STATE:`) generated during the acceptance test run. ```sh -TF_LOG=DEBUG TF_ACC=1 go test -v ./... -run ^TestAccGithubIssueLabel +TF_LOG=DEBUG make testacc T=TestAccGithubIssueLabel ``` 1. Align the resource's implementation to your test case and observe it pass: ```sh -TF_ACC=1 go test -v ./... -run ^TestAccGithubIssueLabel +make testacc T=TestAccGithubIssueLabel ``` Note that some resources still use a previous format that is incompatible with automated test runs, which depend on using the `skipUnlessMode` helper. When encountering these resources, tests should be rewritten to the latest format. @@ -70,7 +72,7 @@ Also note that there is no build / `terraform init` / `terraform plan` sequence ### Debugging the terraform provider -Println debugging can easily be used to obtain information about how code changes perform. If the `TF_LOG=DEBUG` level is set, calls to `log.Printf("[DEBUG] your message here")` will be printed in the program's output. +Println debugging can easily be used to obtain information about how code changes perform. If the `TF_LOG=DEBUG` level is set, debug messages will be printed. Use `tflog.Debug(ctx, "your message here", map[string]any{...})` for new code. Some existing code still uses `log.Printf("[DEBUG] ...")` — see [ARCHITECTURE.md](ARCHITECTURE.md#logging) for the migration pattern. If a full debugger is desired, VSCode may be used. In order to do so, @@ -88,7 +90,7 @@ If a full debugger is desired, VSCode may be used. In order to do so, Setting a `processId` of 0 allows a dropdown to select the process of the provider. -1. Add a sleep call (e.g. `time.Sleep(10 * time.Second)`) in the [`func providerConfigure(p *schema.Provider) schema.ConfigureFunc`](https://github.com/integrations/terraform-provider-github/blob/cec7e175c50bb091feecdc96ba117067c35ee351/github/provider.go#L274C1-L274C64) before the immediate `return` call. This will allow time to connect the debugger while the provider is initializing, before any critical logic happens. +1. Add a sleep call (e.g. `time.Sleep(10 * time.Second)`) in `providerConfigure` (in `github/provider.go`) before the immediate `return` call. This will allow time to connect the debugger while the provider is initializing, before any critical logic happens. 2. Build the terraform provider with debug flags enabled and copy it to the appropriate bin folder with a command like `go build -gcflags="all=-N -l" -o ~/go/bin/`. @@ -104,7 +106,7 @@ Manual testing should be performed on each PR opened in order to validate the pr ### Using a local version of the provider -Build the provider and specify the output directory: +Build the provider with debug flags for attaching a debugger: ```sh go build -gcflags="all=-N -l" -o ~/go/bin/ @@ -179,6 +181,7 @@ export GH_TEST_EXTERNAL_USER2= # Configure values for the enterprise under test export GH_TEST_ENTERPRISE_EMU_GROUP_ID= +export GITHUB_ENTERPRISE_SLUG= # Configure test options export GH_TEST_ADVANCED_SECURITY= From 87de13dc13342892cd93c3c06ded82aa1be10ea8 Mon Sep 17 00:00:00 2001 From: Timo Sand Date: Mon, 16 Feb 2026 13:53:02 +0200 Subject: [PATCH 4/7] Add guidance for Importer Signed-off-by: Timo Sand --- ARCHITECTURE.md | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index d06d0a9230..1dbf26b059 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -17,6 +17,7 @@ This document serves as a guide for contributors implementing new features and r - [CRUD Function Signatures](#crud-function-signatures) - [Accessing the API Client](#accessing-the-api-client) - [Error Handling](#error-handling) + - [Import](#import) - [State Migrations](#state-migrations) - [Logging](#logging) - [Testing](#testing) @@ -342,6 +343,42 @@ if err := deleteResourceOn404AndSwallow304OtherwiseReturnError(err, d, "resource } ``` +### Import + +Import is registered via the `Importer` field with a `StateContext` function. After import runs, Terraform **automatically calls `Read`** — so the import function's only job is to set enough state for `Read` to succeed. Do not duplicate `Read` logic in the import function. + +For resources with a single-part ID, the default passthrough importer is often sufficient: + +```go +Importer: &schema.ResourceImporter{ + StateContext: schema.ImportStatePassthroughContext, +}, +``` + +For resources with composite IDs, the import function must parse the user-provided ID and populate any schema attributes that `Read` depends on: + +```go +func resourceGithubExampleImport(ctx context.Context, d *schema.ResourceData, meta any) ([]*schema.ResourceData, error) { + owner, name, err := parseID2(d.Id()) + if err != nil { + return nil, err + } + + // Set attributes that Read needs to make API calls + d.Set("owner", owner) + // Re-build a normalized ID if needed + id, err := buildID(owner, name) + if err != nil { + return nil, err + } + d.SetId(id) + + return []*schema.ResourceData{d}, nil +} +``` + +**Key principle:** Import sets the minimum state required for `Read` to fetch the full resource. `Read` then populates all remaining attributes. + ### State Migrations When adding new fields or changing schema, use `StateUpgraders` (not the deprecated `MigrateState`): From e011ef376439ea2acefc387bb41808c22b97a0fc Mon Sep 17 00:00:00 2001 From: Timo Sand Date: Sun, 22 Feb 2026 11:20:54 +0200 Subject: [PATCH 5/7] Fix linter warnings Signed-off-by: Timo Sand --- github/resource_github_issue_labels_test.go | 4 ++-- github/resource_github_repository.go | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/github/resource_github_issue_labels_test.go b/github/resource_github_issue_labels_test.go index 9465eb0d68..cf0ad85520 100644 --- a/github/resource_github_issue_labels_test.go +++ b/github/resource_github_issue_labels_test.go @@ -85,13 +85,13 @@ func testAccGithubIssueLabelsConfig(repoName string, labels []map[string]any) st if labels != nil { var dynamic strings.Builder for _, label := range labels { - dynamic.WriteString(fmt.Sprintf(` + fmt.Fprintf(&dynamic, ` label { name = "%s" color = "%s" description = "%s" } - `, label["name"], label["color"], label["description"])) + `, label["name"], label["color"], label["description"]) } resource = fmt.Sprintf(` diff --git a/github/resource_github_repository.go b/github/resource_github_repository.go index fe7db447fc..06c97ff6dd 100644 --- a/github/resource_github_repository.go +++ b/github/resource_github_repository.go @@ -631,7 +631,7 @@ func resourceGithubRepositoryObject(d *schema.ResourceData) *github.Repository { // only configure allow forking if repository is not public if visibility != "public" && (d.IsNewResource() || d.HasChange("allow_forking")) { - if allowForking, ok := d.GetOkExists("allow_forking"); ok { //nolint:staticcheck,SA1019 // We sometimes need to use GetOkExists for booleans + if allowForking, ok := d.GetOkExists("allow_forking"); ok { //nolint:staticcheck //SA1019 // We sometimes need to use GetOkExists for booleans if val, ok := allowForking.(bool); ok { repository.AllowForking = github.Ptr(val) } @@ -1001,7 +1001,7 @@ func resourceGithubRepositoryUpdate(ctx context.Context, d *schema.ResourceData, } if d.IsNewResource() || d.HasChange("vulnerability_alerts") { - if v, ok := d.GetOkExists("vulnerability_alerts"); ok { //nolint:staticcheck,SA1019 // We sometimes need to use GetOkExists for booleans + if v, ok := d.GetOkExists("vulnerability_alerts"); ok { //nolint:staticcheck //SA1019 // We sometimes need to use GetOkExists for booleans if val, ok := v.(bool); ok { err := updateVulnerabilityAlerts(ctx, client, owner, repoName, val) if err != nil { From da53fc56ceb50c8fa893abc56efd28b84fa1f149 Mon Sep 17 00:00:00 2001 From: Timo Sand Date: Sun, 22 Feb 2026 11:21:10 +0200 Subject: [PATCH 6/7] Mark old `parse*ID` and `build*ID` funcs as deprecated Signed-off-by: Timo Sand --- github/util.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/github/util.go b/github/util.go index ac22fd884c..fe3d16f12d 100644 --- a/github/util.go +++ b/github/util.go @@ -142,7 +142,8 @@ func validateValueFunc(values []string) schema.SchemaValidateDiagFunc { } // return the pieces of id `left:right` as left, right. -// @deprecated: use parseID2 instead. +// +// Deprecated: use parseID2 instead. func parseTwoPartID(id, left, right string) (string, string, error) { parts := strings.SplitN(id, ":", 2) if len(parts) != 2 { @@ -153,12 +154,15 @@ func parseTwoPartID(id, left, right string) (string, string, error) { } // format the strings into an id `a:b`. +// +// Deprecated: use buildID instead. func buildTwoPartID(a, b string) string { return fmt.Sprintf("%s:%s", a, b) } // return the pieces of id `left:center:right` as left, center, right. -// @deprecated: use parseID3 instead. +// +// Deprecated: Use parseID3 instead. func parseThreePartID(id, left, center, right string) (string, string, string, error) { parts := strings.SplitN(id, ":", 3) if len(parts) != 3 { @@ -169,6 +173,8 @@ func parseThreePartID(id, left, center, right string) (string, string, string, e } // format the strings into an id `a:b:c`. +// +// Deprecated: use buildID instead. func buildThreePartID(a, b, c string) string { return fmt.Sprintf("%s:%s:%s", a, b, c) } From 72d7098fa381bf9b86cacc968a5bd4764504e26a Mon Sep 17 00:00:00 2001 From: Timo Sand Date: Sun, 22 Feb 2026 11:59:54 +0200 Subject: [PATCH 7/7] Update docs files Signed-off-by: Timo Sand --- ARCHITECTURE.md | 69 +++++++++++++++++++++++++++++++++++++++++-------- CONTRIBUTING.md | 4 +-- MAINTAINERS.md | 4 +++ 3 files changed, 64 insertions(+), 13 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 1dbf26b059..5bf228eca0 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -65,7 +65,7 @@ terraform-provider-github/ │ └── transport.go # Custom HTTP transport with ETag caching │ ├── ARCHITECTURE.md # This file - implementation guide -├── MAINTAINERS.md # Maintainers, decision log, contributors +├── MAINTAINERS.md # Maintainers and contributors └── CONTRIBUTING.md # How to contribute ``` @@ -301,7 +301,7 @@ func resourceGithubExampleDelete(ctx context.Context, d *schema.ResourceData, me ```go func resourceExampleRead(ctx context.Context, d *schema.ResourceData, m any) diag.Diagnostics { meta := m.(*Owner) - // REST API client (go-github v82) + // REST API client (go-github) client := meta.v3client // GraphQL client (for queries not available in REST) @@ -326,7 +326,7 @@ if err != nil { var ghErr *github.ErrorResponse if errors.As(err, &ghErr) { if ghErr.Response.StatusCode == http.StatusNotFound { - log.Printf("[INFO] Removing %s from state because it no longer exists", name) + tflog.Info(ctx, "Removing resource from state because it no longer exists", map[string]any{"name": name}) d.SetId("") return nil } @@ -446,10 +446,21 @@ func resourceExampleCreate(ctx context.Context, d *schema.ResourceData, meta any ### Test Structure +Use `ConfigStateChecks` for post-apply state assertions and `ConfigPlanChecks` for plan-level assertions (e.g., verifying `ForceNew` triggers). These replace the legacy `Check:` + `resource.ComposeTestCheckFunc` pattern. + ```go +import ( + "github.com/hashicorp/terraform-plugin-testing/compare" + "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/hashicorp/terraform-plugin-testing/knownvalue" + "github.com/hashicorp/terraform-plugin-testing/plancheck" + "github.com/hashicorp/terraform-plugin-testing/statecheck" + "github.com/hashicorp/terraform-plugin-testing/tfjsonpath" +) + func TestAccGithubExample(t *testing.T) { - - t.Run("creates resource without error", func(t *testing.T) { + + t.Run("creates resource without error", func(t *testing.T) { randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) testResourceName := fmt.Sprintf("%s%s", testResourcePrefix, randomID) config := fmt.Sprintf(` @@ -464,9 +475,33 @@ func TestAccGithubExample(t *testing.T) { Steps: []resource.TestStep{ { Config: config, - Check: resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttr( "github_example.test", "name", testResourceName ), - ), + ConfigStateChecks: []statecheck.StateCheck{ + // Verify computed values are populated + statecheck.ExpectKnownValue("github_example.test", tfjsonpath.New("etag"), knownvalue.NotNull()), + statecheck.ExpectKnownValue("github_example.test", tfjsonpath.New("node_id"), knownvalue.NotNull()), + // Compare computed values across resources + statecheck.CompareValuePairs("github_example.test", tfjsonpath.New("repo_id"), "github_repository.test", tfjsonpath.New("repo_id"), compare.ValuesSame()), + }, + }, + }, + }) + }) + + t.Run("forces new when field changes", func(t *testing.T) { + // ... config steps ... + + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnauthenticated(t) }, + ProviderFactories: providerFactories, + Steps: []resource.TestStep{ + {Config: configBefore}, + { + Config: configAfter, + ConfigPlanChecks: resource.ConfigPlanChecks{ + PreApply: []plancheck.PlanCheck{ + plancheck.ExpectResourceAction("github_example.test", plancheck.ResourceActionDestroyBeforeCreate), + }, + }, }, }, }) @@ -474,6 +509,8 @@ func TestAccGithubExample(t *testing.T) { } ``` +> **Legacy pattern:** Existing tests may still use `Check: resource.ComposeTestCheckFunc(...)`. New tests should use `ConfigStateChecks` and `ConfigPlanChecks` instead. See `data_source_github_ip_ranges_test.go` for a real-world example. + ### Test Modes Use `skipUnauthenticated(t)`, `skipUnlessHasOrgs(t)`, `skipUnlessHasPaidOrgs(t)`, `skipUnlessEnterprise(t)`, `skipUnlessMode(t, testModes...)` functions to run tests in appropriate contexts: @@ -537,10 +574,8 @@ The following resources are deprecated and will be removed in future versions: ### Pending go-github Updates -Several features are blocked waiting for go-github v68+: +The following features are blocked waiting for upstream changes in [google/go-github#3364](https://github.com/google/go-github/issues/3364) (adds `assignment`, `parent_team_id`, `parent_team_slug` fields): -- `data_source_github_organization_repository_role.go:56` -- `resource_github_organization_repository_role.go:102` - `data_source_github_organization_role_users.go:41` - `data_source_github_organization_role_teams.go:51` @@ -564,6 +599,10 @@ Several features are blocked waiting for go-github v68+: | `expandStringList([]any)` | Convert to `[]string` | | `flattenStringList([]string)` | Convert to `[]any` | | `deleteResourceOn404AndSwallow304OtherwiseReturnError(...)` | Handle 404/304 responses | +| `diffRepository` | `CustomizeDiffFunc`: force replacement on repo ID change | +| `diffSecret` | `CustomizeDiffFunc`: detect remote secret drift via timestamps | +| `diffSecretVariableVisibility` | `CustomizeDiffFunc`: validate `selected_repository_ids` vs `visibility` | +| `diffTeam` | `CustomizeDiffFunc`: force new resource on team ID change | ### Naming Conventions @@ -614,6 +653,14 @@ Several features are blocked waiting for go-github v68+: **Reference:** +#### Migrate to `terraform-plugin-testing` + +**Decision:** Migrate from the SDK testing package (`github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource`) to `terraform-plugin-testing` (`github.com/hashicorp/terraform-plugin-testing`). Use `ConfigStateChecks` and `ConfigPlanChecks` as the preferred assertion patterns, replacing `Check:` + `resource.ComposeTestCheckFunc`. + +**Rationale:** `terraform-plugin-testing` is the standalone testing framework that decouples test utilities from the SDK. `ConfigStateChecks` and `ConfigPlanChecks` provide type-safe, composable assertions with better error messages. + +**Reference:** + #### No Local Git CLI Support **Decision:** Do not support using local git CLI to operate on repositories; use purely API operations. diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0116ebddf8..c96365f96c 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -53,8 +53,8 @@ Once you have the repository cloned, there's a couple of additional steps you'll ### Local Development Iteration -1. Write a test describing what you will fix. See [`github_label`](./github/resource_github_issue_label_test.go) for an example format. -2. Run your test and observe it fail. Enabling debug output allows for observing the underlying requests and responses made as well as viewing state (search `STATE:`) generated during the acceptance test run. +1. Write a test describing what you will fix. See [`github_ip_ranges`](./github/data_source_github_ip_ranges_test.go) for an example using the preferred `ConfigStateChecks` pattern, and [ARCHITECTURE.md](ARCHITECTURE.md#test-structure) for full guidance. +2. Run your test and observe it fail. Enabling debug output allows for observing the underlying requests and responses made during the acceptance test run. ```sh TF_LOG=DEBUG make testacc T=TestAccGithubIssueLabel diff --git a/MAINTAINERS.md b/MAINTAINERS.md index 60e2bae444..cfeb5d618e 100644 --- a/MAINTAINERS.md +++ b/MAINTAINERS.md @@ -17,3 +17,7 @@ This document lists the current maintainers of the Terraform Provider for GitHub | Name | GitHub | | --------------- | -------------------------------------------- | | Keegan Campbell | [@kfcampbell](https://github.com/kfcampbell) | + +## Contributors + +See the full list of contributors on [GitHub](https://github.com/integrations/terraform-provider-github/graphs/contributors).