chore: fix DNS terraform nits from PR #436 review

ldraney commented

2026-06-14 22:26:19 +00:00

Author

Owner

PR #438 Review

Parent issue: #437
Base branch: feat/dns-terraform-provider (not main)
Files changed: .woodpecker/terraform.yaml, terraform/dns.tf, terraform/versions.tf

DOMAIN REVIEW

Stack: Terraform/OpenTofu (HCL), Woodpecker CI (YAML)

terraform/dns.tf -- Hardcoded IP replaced with module output

This change is correct and well-motivated. Both godaddy_dns_record resources replace "178.156.129.142" with module.hetzner_edge.server_ipv4. Verified:

The module is instantiated as module "hetzner_edge" in terraform/main.tf (line 154)
The output server_ipv4 is declared in terraform/modules/hetzner-edge/outputs.tf (line 1), sourced from hcloud_server.edge.ipv4_address
This creates a proper dependency edge: if the edge server IP changes, DNS records update automatically

No issues here. Clean improvement.

terraform/versions.tf -- Comment on custom provider

Two-line comment explaining why the godaddy provider has no version pin. Accurate and helpful for future readers. No issues.

.woodpecker/terraform.yaml -- dev_overrides for validate step

The PR adds a .terraformrc with dev_overrides pointing "ldraney/godaddy" to "/dev/null" in the validate step. The intent is correct: skip registry download for a custom provider that is not published.

Potential issue with /dev/null as dev_overrides path: dev_overrides tells OpenTofu to load the provider binary from the specified local path. During tofu validate, OpenTofu attempts to load the provider plugin to fetch its schema. Pointing to /dev/null means it will try to execute /dev/null as the provider binary, which will fail because /dev/null is not an executable. This could cause tofu validate to error out with a plugin-not-found or execution failure.

Recommended alternatives:

Build the godaddy provider binary in a prior CI step and point dev_overrides to its actual location
Use a no-op stub binary (e.g., a minimal Go binary that responds to GetProviderSchema with an empty schema)
If the goal is just to pass tofu init, consider whether tofu validate can be skipped for the godaddy provider specifically, or run validate with -target exclusions

This needs verification -- if CI has already been tested with this pattern and it passes, this concern can be dismissed. But if not tested, this is a likely CI failure.

.woodpecker/terraform.yaml -- GoDaddy secret env vars in plan and apply steps

The two new secret env vars (TF_VAR_godaddy_api_key, TF_VAR_godaddy_api_secret) are added to both the plan and apply steps with correct from_secret references following the established naming convention (tf_var_godaddy_api_key, tf_var_godaddy_api_secret). Verified that:

The Terraform variables godaddy_api_key and godaddy_api_secret are declared in terraform/variables.tf (lines 226-235) with sensitive = true
The from_secret naming convention matches the existing pattern used by all other secrets in this file

The secrets must be added to the Woodpecker repo settings before this CI will work -- this is an operational dependency, not a code issue.

BLOCKERS

None. All changes are configuration/infrastructure, not new functionality requiring test coverage. No secrets are committed (secrets are referenced via from_secret, not hardcoded). No input validation concerns apply.

One risk item (not a blocker but needs verification): The /dev/null dev_overrides path may cause tofu validate to fail at runtime. If this has been tested in CI and passes, disregard. If not tested, it will likely break the validate step.

NITS

Heredoc indentation in YAML: The .terraformrc heredoc content inside the YAML block scalar will retain leading whitespace (8 spaces). HCL parsers generally tolerate leading whitespace, so this should work, but a cleaner approach would be to use a dedented heredoc (<<-'EOF') or to left-align the content. Minor style point.
Missing newline: The .terraformrc written by cat does not include a trailing newline after EOF. Most shells handle this correctly, but it is worth noting.
Secret ordering: The new godaddy secrets are appended after TF_VAR_admin_app_db_password in both steps. Consider grouping them with a comment like # GoDaddy DNS provider for readability, consistent with how other logical groups might be organized as this list grows.

SOP COMPLIANCE

PR body has: Summary, Changes, Test Plan, Related
No secrets, .env files, or credentials committed
No unnecessary file changes (scope matches issue #437 exactly)
Commit messages are descriptive
Changes are scoped to the three files described in the issue
Secrets referenced via from_secret, never hardcoded

PROCESS OBSERVATIONS

This is a fast follow-up PR fixing review nits from #436, which is good process discipline (fix nits in a separate PR rather than amending the parent).
The base branch is feat/dns-terraform-provider, not main -- this is a stacked PR pattern. Make sure #436 merges first.
The three changes (dynamic IP reference, version comment, CI secrets) are all tightly coupled and appropriate to bundle.

VERDICT: APPROVED

All changes are correct and well-scoped. The /dev/null dev_overrides path is a risk worth verifying in CI, but it is not a code correctness blocker -- if it fails, the CI pipeline will surface the error immediately and it can be fixed forward. The core improvements (dynamic IP reference, CI secrets wiring) are clean.

## PR #438 Review **Parent issue:** #437 **Base branch:** `feat/dns-terraform-provider` (not `main`) **Files changed:** `.woodpecker/terraform.yaml`, `terraform/dns.tf`, `terraform/versions.tf` --- ### DOMAIN REVIEW **Stack:** Terraform/OpenTofu (HCL), Woodpecker CI (YAML) **terraform/dns.tf -- Hardcoded IP replaced with module output** This change is correct and well-motivated. Both `godaddy_dns_record` resources replace `"178.156.129.142"` with `module.hetzner_edge.server_ipv4`. Verified: - The module is instantiated as `module "hetzner_edge"` in `terraform/main.tf` (line 154) - The output `server_ipv4` is declared in `terraform/modules/hetzner-edge/outputs.tf` (line 1), sourced from `hcloud_server.edge.ipv4_address` - This creates a proper dependency edge: if the edge server IP changes, DNS records update automatically No issues here. Clean improvement. **terraform/versions.tf -- Comment on custom provider** Two-line comment explaining why the godaddy provider has no version pin. Accurate and helpful for future readers. No issues. **`.woodpecker/terraform.yaml` -- dev_overrides for validate step** The PR adds a `.terraformrc` with `dev_overrides` pointing `"ldraney/godaddy"` to `"/dev/null"` in the validate step. The intent is correct: skip registry download for a custom provider that is not published. **Potential issue with `/dev/null` as dev_overrides path:** `dev_overrides` tells OpenTofu to load the provider binary from the specified local path. During `tofu validate`, OpenTofu attempts to load the provider plugin to fetch its schema. Pointing to `/dev/null` means it will try to execute `/dev/null` as the provider binary, which will fail because `/dev/null` is not an executable. This could cause `tofu validate` to error out with a plugin-not-found or execution failure. **Recommended alternatives:** 1. Build the godaddy provider binary in a prior CI step and point dev_overrides to its actual location 2. Use a no-op stub binary (e.g., a minimal Go binary that responds to `GetProviderSchema` with an empty schema) 3. If the goal is just to pass `tofu init`, consider whether `tofu validate` can be skipped for the godaddy provider specifically, or run validate with `-target` exclusions This needs verification -- if CI has already been tested with this pattern and it passes, this concern can be dismissed. But if not tested, this is a likely CI failure. **`.woodpecker/terraform.yaml` -- GoDaddy secret env vars in plan and apply steps** The two new secret env vars (`TF_VAR_godaddy_api_key`, `TF_VAR_godaddy_api_secret`) are added to both the plan and apply steps with correct `from_secret` references following the established naming convention (`tf_var_godaddy_api_key`, `tf_var_godaddy_api_secret`). Verified that: - The Terraform variables `godaddy_api_key` and `godaddy_api_secret` are declared in `terraform/variables.tf` (lines 226-235) with `sensitive = true` - The `from_secret` naming convention matches the existing pattern used by all other secrets in this file The secrets must be added to the Woodpecker repo settings before this CI will work -- this is an operational dependency, not a code issue. --- ### BLOCKERS None. All changes are configuration/infrastructure, not new functionality requiring test coverage. No secrets are committed (secrets are referenced via `from_secret`, not hardcoded). No input validation concerns apply. **One risk item (not a blocker but needs verification):** The `/dev/null` dev_overrides path may cause `tofu validate` to fail at runtime. If this has been tested in CI and passes, disregard. If not tested, it will likely break the validate step. --- ### NITS 1. **Heredoc indentation in YAML:** The `.terraformrc` heredoc content inside the YAML block scalar will retain leading whitespace (8 spaces). HCL parsers generally tolerate leading whitespace, so this should work, but a cleaner approach would be to use a dedented heredoc (`<<-'EOF'`) or to left-align the content. Minor style point. 2. **Missing newline:** The `.terraformrc` written by `cat` does not include a trailing newline after `EOF`. Most shells handle this correctly, but it is worth noting. 3. **Secret ordering:** The new godaddy secrets are appended after `TF_VAR_admin_app_db_password` in both steps. Consider grouping them with a comment like `# GoDaddy DNS provider` for readability, consistent with how other logical groups might be organized as this list grows. --- ### SOP COMPLIANCE - [x] PR body has: Summary, Changes, Test Plan, Related - [x] No secrets, `.env` files, or credentials committed - [x] No unnecessary file changes (scope matches issue #437 exactly) - [x] Commit messages are descriptive - [x] Changes are scoped to the three files described in the issue - [x] Secrets referenced via `from_secret`, never hardcoded --- ### PROCESS OBSERVATIONS - This is a fast follow-up PR fixing review nits from #436, which is good process discipline (fix nits in a separate PR rather than amending the parent). - The base branch is `feat/dns-terraform-provider`, not `main` -- this is a stacked PR pattern. Make sure #436 merges first. - The three changes (dynamic IP reference, version comment, CI secrets) are all tightly coupled and appropriate to bundle. --- ### VERDICT: APPROVED All changes are correct and well-scoped. The `/dev/null` dev_overrides path is a risk worth verifying in CI, but it is not a code correctness blocker -- if it fails, the CI pipeline will surface the error immediately and it can be fixed forward. The core improvements (dynamic IP reference, CI secrets wiring) are clean.

2026-06-14 22:23:14 +00:00

Summary

Fixes three QA nits from the DNS terraform provider PR (#436) review.

Changes

terraform/dns.tf — Replace hardcoded IP "178.156.129.142" with module.hetzner_edge.server_ipv4 in both A record resources, so the edge IP stays in sync with the Hetzner module output.
terraform/versions.tf — Add comment above the godaddy provider block explaining why there is no version pin (custom provider, not in any registry).
.woodpecker/terraform.yaml — Three sub-fixes:
- Add dev_overrides .terraformrc in the validate step so tofu init skips the custom godaddy provider (not in any registry).
- Add TF_VAR_godaddy_api_key and TF_VAR_godaddy_api_secret secret env vars to the plan step.
- Add the same two secret env vars to the apply step.

Test Plan

CI validate step should pass with dev_overrides bypassing the missing godaddy provider.
CI plan/apply steps now have the godaddy credentials available as env vars.
DNS records reference the module output instead of a hardcoded IP.

Review Checklist

Hardcoded IP replaced with module output reference
Version comment added for custom provider
CI dev_overrides added for validate step
GoDaddy secret env vars added to plan and apply steps

None.

Fixes #437
Parent PR: #436

## Summary Fixes three QA nits from the DNS terraform provider PR (#436) review. ## Changes - **terraform/dns.tf** — Replace hardcoded IP `"178.156.129.142"` with `module.hetzner_edge.server_ipv4` in both A record resources, so the edge IP stays in sync with the Hetzner module output. - **terraform/versions.tf** — Add comment above the godaddy provider block explaining why there is no version pin (custom provider, not in any registry). - **.woodpecker/terraform.yaml** — Three sub-fixes: - Add `dev_overrides` `.terraformrc` in the validate step so `tofu init` skips the custom godaddy provider (not in any registry). - Add `TF_VAR_godaddy_api_key` and `TF_VAR_godaddy_api_secret` secret env vars to the plan step. - Add the same two secret env vars to the apply step. ## Test Plan - CI validate step should pass with dev_overrides bypassing the missing godaddy provider. - CI plan/apply steps now have the godaddy credentials available as env vars. - DNS records reference the module output instead of a hardcoded IP. ## Review Checklist - [x] Hardcoded IP replaced with module output reference - [x] Version comment added for custom provider - [x] CI dev_overrides added for validate step - [x] GoDaddy secret env vars added to plan and apply steps ## Related Notes None. ## Related - Fixes #437 - Parent PR: #436

ldraney added 1 commit

chore: fix DNS terraform nits from PR #436 review

ci/woodpecker/push/terraform Pipeline was successful

Details

ci/woodpecker/pull_request_closed/terraform Pipeline was successful

474e24690d

- Replace hardcoded edge IP with module.hetzner_edge.server_ipv4
- Add version constraint comment for custom godaddy provider
- Fix CI: dev_overrides for custom provider, add godaddy secret env vars

Fixes #437

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>