fix: prefer provider block attributes over environment variables

[GideonStowell]

fix: prefer provider block attributes over environment variables

Problem

Configure() seeded host/api_key from DEVHUB_HOST/DEVHUB_API_KEY and only
fell back to the provider-block values when those env vars were empty. This gave
environment variables precedence over explicitly-set provider-block attributes —
the inverse of the Terraform Plugin Framework convention.

Behavior change

• Before: DEVHUB_HOST / DEVHUB_API_KEY override an explicitly-set host /
  api_key in the provider "devhub" block. The block value is used only when the
  env var is empty.
• After: An explicitly-set host / api_key in the provider block wins; the
  env var is used only as a fallback when the attribute is unset/empty.

Justification

This matches the Plugin Framework's canonical Configure example, which seeds from
the env var and then overrides with the config value when set:
https://developer.hashicorp.com/terraform/plugin/framework/providers

⚠️ Breaking behavior note

This is a behavior change for anyone who relied on the old env-wins ordering — e.g.
setting DEVHUB_HOST/DEVHUB_API_KEY in the environment to override a value written
in the provider block. After this change the provider-block value takes precedence.
To keep env-based configuration, leave the corresponding provider-block attribute
unset so the env var is used as the fallback.

Tests

The acceptance test providerConfig previously hardcoded host = "http://localhost:4000"
and api_key = "test", relying on the old env-wins behavior for CI to override them via
DEVHUB_HOST/DEVHUB_API_KEY. Since host is a Required attribute, the block can't be
empty, so it now sources host from DEVHUB_HOST and omits api_key (falling through to
DEVHUB_API_KEY) — preserving the CI-provided host/key under the corrected precedence.

Verification

• gofmt -s, go build ./..., go vet ./... all clean
• go generate ./... produces no generated-docs drift
• Acceptance tests not run here (require TF_ACC + a real Devhub host/secret)

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Fix provider config precedence (provider block overrides env vars)

🐞 Bug fix 🧪 Tests 🕐 10-20 Minutes

AI Description

• Invert Configure() precedence so provider-block host/api_key override DEVHUB_* env vars.
• Use DEVHUB_* as fallback only when provider attributes are unset/empty.
• Update acceptance test config to align with corrected precedence in CI.

Diagram

graph TD
  TF["Terraform Core"] --> CFG["Provider Configure()"] --> MERGE["Resolve host/api_key"] --> CLIENT["Devhub client"]
  PB["Provider block attrs"] --> MERGE
  ENV{{"DEVHUB_* env vars"}} --> MERGE

Loading

High-Level Assessment

The chosen approach (seed from env vars, then override with explicitly-set provider attributes) matches Terraform Plugin Framework conventions and minimizes behavioral surprise. Alternatives like adding an explicit precedence toggle or deprecating env vars would add complexity without clear benefit for this provider.

Files changed (2) +8 / -8

Bug fix (1) +2 / -2

provider.goInvert Configure() precedence: provider attributes override env +2/-2

Invert Configure() precedence: provider attributes override env

• Changes Configure() so non-empty provider-block host/api_key values override DEVHUB_HOST/DEVHUB_API_KEY. Environment variables are now only used when the corresponding attribute is unset/empty, aligning with Terraform Plugin Framework convention.

internal/provider/provider.go

Tests (1) +6 / -6

provider_test.goAdjust acceptance providerConfig to rely on DEVHUB_HOST/DEVHUB_API_KEY fallback +6/-6

Adjust acceptance providerConfig to rely on DEVHUB_HOST/DEVHUB_API_KEY fallback

• Updates the acceptance test provider configuration to source host from DEVHUB_HOST and omit api_key so DEVHUB_API_KEY can populate it. Converts the config to a fmt.Sprintf-generated string and adds required imports.

internal/provider/provider_test.go

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📜 Skill insights (0)

1. Env baked into providerConfig 🐞 Bug ☼ Reliability

Description

internal/provider/provider_test.go now renders providerConfig using os.Getenv("DEVHUB_HOST") at
package init, so a missing/empty DEVHUB_HOST produces host = "" in the test configuration and
breaks local runs that previously defaulted to localhost. This also prevents tests from adjusting
DEVHUB_HOST dynamically during the test run because the value is captured once at init time.

Code

internal/provider/provider_test.go[R14-18]

+var providerConfig = fmt.Sprintf(`
provider "devhub" {
-	host    = "http://localhost:4000"
-	api_key = "test"
+	host = %q
}
-`
-)
+`, os.Getenv("DEVHUB_HOST"))

Evidence

The test config now directly embeds DEVHUB_HOST as the provider-block host value; when that env var
is empty, Terraform config host becomes an empty string. The provider uses env/config precedence to
compute host, then always passes &host into the client, and the client overwrites its default
HostURL with the provided string (even if empty).

internal/provider/provider_test.go[14-19]
internal/provider/provider.go[79-88]
internal/provider/provider.go[104-117]
internal/client/client.go[11-28]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`providerConfig` is initialized once at package load time via `os.Getenv("DEVHUB_HOST")`. When the env var is unset/empty, the generated Terraform config contains `host = ""`, and because the provider passes the resulting `host` through to the client, acceptance tests lose the previous localhost default behavior.

## Issue Context
`providerConfig` is concatenated into multiple acceptance-test configs across `internal/provider/*_test.go`.

## Fix Focus Areas
- internal/provider/provider_test.go[14-19]

Suggested direction:
- Replace the global `var providerConfig = ...` with a function (e.g., `testAccProviderConfig() string`) that reads env vars at call time.
- Provide a safe fallback when `DEVHUB_HOST` is empty (e.g., `http://localhost:4000`, matching prior behavior / client default).
- (Optionally) if you want to require env vars for acceptance tests, do not fallback silently—fail/skip via a PreCheck instead (see next finding).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Tests lack env PreCheck 🐞 Bug ⚙ Maintainability

Description

Acceptance-style tests now omit api_key from providerConfig and rely on DEVHUB_API_KEY being set,
but no acceptance-test PreCheck/skip exists to enforce those required env vars. This causes
confusing failures during go test runs (provider Configure diagnostics) instead of a clear
“missing env” guard or a skip when not running acceptance tests.

Code

internal/provider/provider_test.go[R14-18]

+var providerConfig = fmt.Sprintf(`
provider "devhub" {
-	host    = "http://localhost:4000"
-	api_key = "test"
+	host = %q
}
-`
-)
+`, os.Getenv("DEVHUB_HOST"))

Evidence

providerConfig no longer sets api_key, so Configure depends on DEVHUB_API_KEY; Configure explicitly
errors when api_key is empty. The acceptance tests use resource.TestCase without any PreCheck to
validate/skip based on env vars, so missing env var setups will fail during provider configuration.

internal/provider/provider_test.go[14-22]
internal/provider/provider.go[79-98]
internal/provider/devhub_dashboard_resource_test.go[11-20]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Acceptance tests rely on `DEVHUB_HOST`/`DEVHUB_API_KEY` being set (providerConfig omits `api_key`, and `Configure` errors when the env var is missing), but the tests don’t define a `PreCheck` (or `TestMain`) to verify required env vars / TF_ACC and to skip or fail fast.

## Issue Context
Multiple `resource.TestCase{...}` definitions under `internal/provider/*_test.go` do not provide `PreCheck`, so they will attempt to run with missing env vars and fail later in provider configuration.

## Fix Focus Areas
- internal/provider/provider_test.go[14-22]
- internal/provider/devhub_dashboard_resource_test.go[11-20]

Suggested direction:
- Add a `testAccPreCheck(t *testing.T)` helper that validates `TF_ACC` (if you use that convention) and required env vars (`DEVHUB_HOST`, `DEVHUB_API_KEY`).
- Wire it into each `resource.TestCase` via `PreCheck: func() { testAccPreCheck(t) }`, or implement `TestMain` to gate the entire package’s acceptance tests.
- Optionally, keep a local-dev fallback for host/api_key when TF_ACC is not set, if these are intended to be runnable without a real endpoint.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---