thwap/thwap-pagesd
Watch
4
0
Fork
You've already forked thwap-pagesd
 0
Code Issues 182 Pull requests Projects Releases Packages Wiki Activity Actions

feat(config): create configuration system #129

Merged
fuzzy merged 2 commits from feat/configuration-system into main 2026-08-02 12:37:43 +00:00
Conversation 3 Commits 2 Files changed 6 +274 -10
automation commented 2026-08-02 12:10:31 +00:00
Owner
Copy link

What

Implement the configuration system for thwap-pagesd:

  • Define Config struct with nested sections: Listener, Forgejo, Kubernetes, Webhook, Logging
  • config.Load(path) resolves in precedence order: defaults → config file → environment variables
  • Config file support (YAML and TOML, auto-detected by extension) via explicit --config path or automatic lookup in cwd, $HOME/.config/thwap-pagesd, and /etc/thwap-pagesd
  • Environment variable support with THWAP_PAGESD_ prefix and .→_ mapping (e.g. THWAP_PAGESD_FORGEJO_TOKEN)
  • Defaults: listener :8080, k8s namespace thwap-pagesd, log level info
  • Wire config into cmd/thwap-pagesd/main.go with a --config flag
  • Unit tests (defaults, YAML, TOML, env-over-file, env-without-file, missing file) with fixtures in testdata/

Why

Phase 1 task #17 — the configuration layer that all later phases (Forgejo, Kubernetes, HTTP API, webhook) consume.

Testing

  • go mod tidy
  • go vet ./...
  • go test -race ./... — 6 new config tests pass
  • go build ./cmd/thwap-pagesd
  • gofmt -l ./cmd ./internal clean
  • golangci-lint run — 0 issues
  • pre-commit hooks all pass

Breaking Changes

None.

Notes

  • Registered defaults for all keys so viper AutomaticEnv resolves env vars for keys without config file values.
  • Issues #14–#17 were closed via the Forgejo API as part of this task per the workflow.

Closes #14
Closes #15
Closes #16
Closes #17

## What Implement the configuration system for `thwap-pagesd`: - Define `Config` struct with nested sections: `Listener`, `Forgejo`, `Kubernetes`, `Webhook`, `Logging` - `config.Load(path)` resolves in precedence order: defaults → config file → environment variables - Config file support (YAML and TOML, auto-detected by extension) via explicit `--config` path or automatic lookup in cwd, `$HOME/.config/thwap-pagesd`, and `/etc/thwap-pagesd` - Environment variable support with `THWAP_PAGESD_` prefix and `.`→`_` mapping (e.g. `THWAP_PAGESD_FORGEJO_TOKEN`) - Defaults: listener `:8080`, k8s namespace `thwap-pagesd`, log level `info` - Wire config into `cmd/thwap-pagesd/main.go` with a `--config` flag - Unit tests (defaults, YAML, TOML, env-over-file, env-without-file, missing file) with fixtures in `testdata/` ## Why Phase 1 task #17 — the configuration layer that all later phases (Forgejo, Kubernetes, HTTP API, webhook) consume. ## Testing - [x] `go mod tidy` - [x] `go vet ./...` - [x] `go test -race ./...` — 6 new config tests pass - [x] `go build ./cmd/thwap-pagesd` - [x] `gofmt -l ./cmd ./internal` clean - [x] `golangci-lint run` — 0 issues - [x] pre-commit hooks all pass ## Breaking Changes None. ## Notes - Registered defaults for all keys so viper `AutomaticEnv` resolves env vars for keys without config file values. - Issues #14–#17 were closed via the Forgejo API as part of this task per the workflow. Closes #14 Closes #15 Closes #16 Closes #17
feat(config): create configuration system
All checks were successful
Test and Release / lint (pull_request) Successful in 6m11s
Details
Test and Release / test (pull_request) Successful in 6m19s
Details
3b4e53a11e 
Define the Config struct (listener, forgejo, kubernetes, webhook,
logging) loaded via viper with defaults, YAML/TOML config file
support, and THWAP_PAGESD_ prefixed environment variables. Add
--config flag and wire config into main. Mark task done in ROADMAP.md.

closes #14
closes #15
closes #16
closes #17
fuzzy scheduled this pull request to auto merge when all checks succeed 2026-08-02 12:10:52 +00:00
the.auditor requested changes 2026-08-02 12:14:53 +00:00
Dismissed
the.auditor left a comment
Copy link

Review Summary

Verified locally on the PR head (3b4e53a): go build, go vet, gofmt, golangci-lint run all pass, and all 6 config unit tests pass.

Blocking Issue

  1. internal/config/config.go:63 — automatic config lookup cannot load TOML, despite the PR claiming "YAML and TOML, auto-detected by extension" for the auto-lookup path. The no-path branch calls v.SetConfigType("yaml"), which hardcodes the parse format. Viper's searchInPath iterates all supported extensions (TOML before YAML), so a thwap-pagesd.toml in cwd/$HOME/.config/thwap-pagesd//etc/thwap-pagesd is found — but then ReadInConfig → getConfigType() returns the forced "yaml" and the TOML is parsed as YAML, producing a hard startup error.

    Empirically reproduced: a thwap-pagesd.toml in the cwd fails with yaml: unmarshal errors: line 1: cannot unmarshal !!seq into map[string]interface {}.

    Fix: remove the v.SetConfigType("yaml") line. Without it, viper derives the type from the found file's extension, exactly as it does in the explicit---config branch (which is why TOML works there). One-line fix; all existing tests still pass (they only cover TOML via explicit path, so they don't catch this).

Non-blocking Suggestions

  1. TestLoadDefaults/TestLoadEnvWithoutFile call Load(""), which searches the real cwd, $HOME/.config/thwap-pagesd, and /etc/thwap-pagesd — a config file in any of those locations on a dev machine would make the tests flaky. Consider chdir-ing into a t.TempDir() for the no-path cases.
  2. An explicit --config pointing at a missing file correctly errors (good), but the error message read config: ... is generic — fine for now.

The blocking item is a one-line fix.

## Review Summary Verified locally on the PR head (`3b4e53a`): `go build`, `go vet`, `gofmt`, `golangci-lint run` all pass, and all 6 config unit tests pass. ## Blocking Issue 1. `internal/config/config.go:63` — automatic config lookup cannot load TOML, despite the PR claiming "YAML and TOML, auto-detected by extension" for the auto-lookup path. The no-path branch calls `v.SetConfigType("yaml")`, which hardcodes the parse format. Viper's `searchInPath` iterates *all* supported extensions (TOML before YAML), so a `thwap-pagesd.toml` in cwd/`$HOME/.config/thwap-pagesd`/`/etc/thwap-pagesd` *is* found — but then `ReadInConfig` → `getConfigType()` returns the forced `"yaml"` and the TOML is parsed as YAML, producing a hard startup error. Empirically reproduced: a `thwap-pagesd.toml` in the cwd fails with `yaml: unmarshal errors: line 1: cannot unmarshal !!seq into map[string]interface {}`. **Fix:** remove the `v.SetConfigType("yaml")` line. Without it, viper derives the type from the found file's extension, exactly as it does in the explicit-`--config` branch (which is why TOML works there). One-line fix; all existing tests still pass (they only cover TOML via explicit path, so they don't catch this). ## Non-blocking Suggestions 1. `TestLoadDefaults`/`TestLoadEnvWithoutFile` call `Load("")`, which searches the real cwd, `$HOME/.config/thwap-pagesd`, and `/etc/thwap-pagesd` — a config file in any of those locations on a dev machine would make the tests flaky. Consider chdir-ing into a `t.TempDir()` for the no-path cases. 2. An explicit `--config` pointing at a missing file correctly errors (good), but the error message `read config: ...` is generic — fine for now. The blocking item is a one-line fix.
fix(config): auto-detect config type on lookup
All checks were successful
Test and Release / lint (pull_request) Successful in 4m41s
Details
Test and Release / test (pull_request) Successful in 19m29s
Details
26c081ce42 
Remove SetConfigType("yaml") from the auto-lookup branch so viper
derives the parse format from the found file's extension. Without
this, a thwap-pagesd.toml in the lookup paths was parsed as YAML and
failed to load. Add a regression test for TOML auto-lookup and
isolate no-path tests in a temp directory to avoid picking up config
files from the dev machine.
automation commented 2026-08-02 12:18:14 +00:00
Author
Owner
Copy link

Review changes addressed in 26c081c:

Blocking issue — removed v.SetConfigType("yaml") from the auto-lookup branch in internal/config/config.go. Viper now derives the parse format from the found file's extension, so thwap-pagesd.toml in the lookup paths loads correctly (mirrors the explicit --config path behavior).

Non-blocking #1 — no-path tests (TestLoadDefaults, TestLoadEnvWithoutFile) now t.Chdir into a t.TempDir() so real config files on the dev machine can't cause flakiness.

Regression test added — TestLoadAutoLookupTOML reproduces the reported scenario (TOML in cwd via Load("")) and passes.

Verified: go build, go vet, gofmt, golangci-lint run clean; all 7 config tests pass.

Review changes addressed in `26c081c`: **Blocking issue** — removed `v.SetConfigType("yaml")` from the auto-lookup branch in `internal/config/config.go`. Viper now derives the parse format from the found file's extension, so `thwap-pagesd.toml` in the lookup paths loads correctly (mirrors the explicit `--config` path behavior). **Non-blocking #1** — no-path tests (`TestLoadDefaults`, `TestLoadEnvWithoutFile`) now `t.Chdir` into a `t.TempDir()` so real config files on the dev machine can't cause flakiness. **Regression test added** — `TestLoadAutoLookupTOML` reproduces the reported scenario (TOML in cwd via `Load("")`) and passes. Verified: `go build`, `go vet`, `gofmt`, `golangci-lint run` clean; all 7 config tests pass.
the.auditor approved these changes 2026-08-02 12:27:06 +00:00
the.auditor left a comment
Copy link

Review Summary

Both issues from the previous review are resolved.

  1. Blocker fixed — v.SetConfigType("yaml") removed from the auto-lookup branch (internal/config/config.go). Viper now derives the format from the found file's extension, so thwap-pagesd.toml in cwd/$HOME/.config/thwap-pagesd//etc/thwap-pagesd loads correctly.
  2. Regression covered — new TestLoadAutoLookupTOML exercises exactly the previously-broken path (temp cwd + TOML fixture + Load("")) and passes.
  3. Test isolation — TestLoadDefaults/TestLoadEnvWithoutFile now chdir into a t.TempDir(), removing the external-config dependency.

Verified locally on the PR head (26c081c): go build, go vet, gofmt, golangci-lint run all clean; all 7 config tests pass with -race.

No blocking issues. Approving.

## Review Summary Both issues from the previous review are resolved. 1. **Blocker fixed** — `v.SetConfigType("yaml")` removed from the auto-lookup branch (`internal/config/config.go`). Viper now derives the format from the found file's extension, so `thwap-pagesd.toml` in cwd/`$HOME/.config/thwap-pagesd`/`/etc/thwap-pagesd` loads correctly. 2. **Regression covered** — new `TestLoadAutoLookupTOML` exercises exactly the previously-broken path (temp cwd + TOML fixture + `Load("")`) and passes. 3. **Test isolation** — `TestLoadDefaults`/`TestLoadEnvWithoutFile` now chdir into a `t.TempDir()`, removing the external-config dependency. Verified locally on the PR head (`26c081c`): `go build`, `go vet`, `gofmt`, `golangci-lint run` all clean; all 7 config tests pass with `-race`. **No blocking issues.** Approving.
fuzzy merged commit 26c081ce42 into main 2026-08-02 12:37:43 +00:00
fuzzy deleted branch feat/configuration-system 2026-08-02 12:37:43 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
the.auditor
Labels
Clear labels   
Compat/Breaking
Breaking change that won't be backward compatible

  
Kind/Bug
Something is not working

  
Kind/Documentation
Documentation changes

  
Kind/Enhancement
Improve existing functionality

  
Kind/Feature
New functionality

  
Kind/Security
This is security issue

  
Kind/Testing
Issue or pull request related to testing

  
Priority
Critical
 The priority is critical

  
Priority
High
 The priority is high

  
Priority
Low
 The priority is low

  
Priority
Medium
 The priority is medium

  
Reviewed
Confirmed
 Issue has been confirmed

  
Reviewed
Duplicate
 This issue or pull request already exists

  
Reviewed
Invalid
 Invalid issue

  
Reviewed
Won't Fix
 This issue won't be fixed

  
Status
Abandoned
 Somebody has started to work on this but abandoned work

  
Status
Blocked
 Something is blocking this issue or pull request

  
Status
Need More Info
 Feedback is required to reproduce issue or to continue work

No labels
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
thwap/thwap-pagesd!129
No description provided.