Ensure Environment Variable Configurations are Documented #304
Conversation
WalkthroughReflection-driven config defaults replace manual Viper wiring; a generator tool emits docs/environment.md from Config struct tags; README links to generated docs and shows Docker usage; application startup now uses Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as LoadConfig()
participant Viper
participant Setter as setDefaultConfig()
participant Reflect as reflect
participant Config
Caller->>Viper: New Viper, SetEnvPrefix("FULMINE")
Caller->>Setter: setDefaultConfig(v)
Setter->>Reflect: Inspect Config struct tags (mapstructure/envDefault/envInfo)
Reflect-->>Setter: field metadata
Setter->>Viper: v.SetDefault(key, default) & v.BindEnv(key)
Caller->>Viper: v.Unmarshal(&Config)
Viper-->>Config: populated fields
Caller->>Config: initDb()
Caller->>Config: deriveLnConfig()
note right of Config: tools/gen-env-doc reads same tags\nand writes docs/environment.md
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…d simple testing of default variables
…st to skip Datadir
# Conflicts: # cmd/fulmine/main.go # internal/config/config.go
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/env-doc-check.yaml (1)
13-19: Update GitHub Actions to v4.The
actions/checkout@v3andactions/setup-go@v3actions are outdated. GitHub recommends using v4 for improved security and Node.js 20 compatibility.- - uses: actions/checkout@v3 + - uses: actions/checkout@v4 with: fetch-depth: 1 - - uses: actions/setup-go@v3 + - uses: actions/setup-go@v4 with: go-version: '>=1.24.6' cache: truetools/gen-env-doc/main.go (1)
36-41: Consider more robust path handling for the output directory.The hardcoded relative paths
../../docsand../../docs/environment.mdassume the tool is run fromtools/gen-env-doc/. While this works correctly when invoked viago generatefrom the expected location, it could be fragile if run manually from a different directory.Consider one of these approaches for better robustness:
Option 1: Accept output path as a flag
import "flag" func main() { outputPath := flag.String("output", "docs/environment.md", "output markdown file path") flag.Parse() // ... generate markdown ... dir := filepath.Dir(*outputPath) if err := os.MkdirAll(dir, 0o755); err != nil { panic(err) } if err := os.WriteFile(*outputPath, []byte(md), 0o644); err != nil { panic(err) } }Option 2: Determine paths relative to the Go module root
import ( "os/exec" "path/filepath" "strings" ) func main() { // Get module root out, err := exec.Command("go", "env", "GOMOD").Output() if err != nil { panic(err) } modFile := strings.TrimSpace(string(out)) moduleRoot := filepath.Dir(modFile) docsDir := filepath.Join(moduleRoot, "docs") outputPath := filepath.Join(docsDir, "environment.md") // ... rest of the code ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/env-doc-check.yaml(1 hunks)README.md(1 hunks)cmd/fulmine/main.go(1 hunks)docs/environment.md(1 hunks)internal/config/config.go(6 hunks)tools/gen-env-doc/main.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tools/gen-env-doc/main.go (1)
internal/config/config.go (1)
Config(27-58)
🪛 actionlint (1.7.8)
.github/workflows/env-doc-check.yaml
13-13: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
16-16: the runner of "actions/setup-go@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration tests
🔇 Additional comments (12)
docs/environment.md (1)
1-29: LGTM! Documentation structure is clear and comprehensive.The generated environment variable documentation covers all configuration options with appropriate defaults, types, and descriptions. The warning about not editing manually is helpful for maintainers.
internal/config/config.go (5)
27-58: LGTM! Config struct is well-structured with appropriate tags.The use of
mapstructure,envDefault, andenvInfotags enables reflection-based configuration binding and documentation generation. The unexportedlnConnectionOptsfield with a public accessor maintains good encapsulation.
60-93: LGTM! LoadConfig flow is clean and well-organized.The initialization sequence is logical, with proper error handling at each step. Creating a new Viper instance rather than using the global one improves testability.
95-163: LGTM! Lightning config derivation logic is thorough.The method properly validates mutual exclusion of LND/CLN configurations, handles both connect-scheme URLs and standard URLs with datadir requirements, and validates URL formats where appropriate.
169-171: LGTM!Clean accessor method for the encapsulated LN connection options.
241-259: LGTM! Reflection-based config binding is well-implemented.The function correctly iterates over exported fields, extracts tags, sets defaults when present, and binds environment variables. Skipping unexported fields prevents panics on inaccessible fields.
cmd/fulmine/main.go (1)
125-125: LGTM!The change correctly uses the new
GetLnConnectionOpts()accessor, aligning with the encapsulation pattern introduced in the config refactor.README.md (1)
66-66: LGTM!Linking to the generated
docs/environment.mdinstead of maintaining a duplicate table is a good approach—keeps the README concise and ensures environment variable documentation stays in sync with the code..github/workflows/env-doc-check.yaml (1)
20-21: LGTM! Generated documentation validation is a good CI practice.Running
go generateand checking for uncommitted changes ensuresdocs/environment.mdstays synchronized with theConfigstruct.tools/gen-env-doc/main.go (3)
1-2: LGTM!The build constraints correctly mark this as a generator tool that should be excluded from regular builds.
6-12: LGTM!Imports are appropriate for the reflection-based documentation generator.
14-20: LGTM!The reflection setup and markdown table header are correctly structured. The warning about manual edits is helpful for maintainers.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
tools/gen-env-doc/main.go (2)
14-43: Reflection-based env docs generation and tag validation look solid (minor wording nit).The reflection over
config.ConfigwithIsExported()plus explicit checks formapstructureandenvInfotags is a good way to keep env docs in sync and fail fast when new fields are added without proper tags. The generated table columns (Variable, Default, Type, Description) line up cleanly with the struct tags and intended usage.Very small nit: the header text
"Generated from \config Structure`"is slightly odd; consider changing it to something like"Generated from `config.Config` struct"` for clarity.
45-50: Make the output path more resilient to different working directories.Using hard-coded
../../docsassumes this tool is always executed frominternal/configviago generate. If someone runsgo run tools/gen-env-doc/main.gofrom the repo root, it may write outside the repo tree.Consider deriving the docs path from the current working directory (e.g.,
filepath.Abs+filepath.Join) or accepting the output path as an argument, with a default matching the currentgo:generateusage.internal/config/config.go (3)
95-155: LN connection derivation covers key combinations; consider ensuring tests exhaust cases.The standalone
deriveLnConfigcorrectly:
- Normalizes datadir paths.
- Rejects simultaneous LND/CLN URLs or datadirs.
- Handles
lndconnect://andclnconnect://specially (no datadir required).- Requires corresponding datadirs for non-
*connectURLs.- Validates non-
*connectURLs viautils.ValidateURLbefore constructingLnConnectionOpts.Given the number of configuration combinations here, it would be worth having (or extending) a table-driven test suite that exercises all branches (no URLs, both URLs, only LND, only CLN, with/without datadirs, invalid URLs,
*connectschemes, stray datadir values). If such tests don’t already exist, I’d recommend adding them.
175-197: DB type validation looks good; consider also creating the default datadir.The
supportedDbTypecheck provides clear feedback for unsupported DB backends, and the non-default datadir path is normalized and created viamakeDirectoryIfNotExists.One potential improvement: in the
c.Datadir == "fulmine"branch you compute the OS-specific app datadir but don’t ensure the directory exists, while theelsebranch does. For symmetry and first-run ergonomics, you may want to runmakeDirectoryIfNotExistsin both branches:- if c.Datadir == "fulmine" { - c.Datadir = appDatadir("fulmine", false) - } else { - datadir := cleanAndExpandPath(c.Datadir) - if err := makeDirectoryIfNotExists(datadir); err != nil { - return fmt.Errorf("failed to create data directory: %w", err) - } - c.Datadir = datadir - } + if c.Datadir == "fulmine" { + datadir := appDatadir("fulmine", false) + if err := makeDirectoryIfNotExists(datadir); err != nil { + return fmt.Errorf("failed to create data directory: %w", err) + } + c.Datadir = datadir + } else { + datadir := cleanAndExpandPath(c.Datadir) + if err := makeDirectoryIfNotExists(datadir); err != nil { + return fmt.Errorf("failed to create data directory: %w", err) + } + c.Datadir = datadir + }
244-262: Add explicit validation for missingmapstructuretags insetDefaultConfig.BindEnv with an empty key does not error—it binds using the empty string as the config key, producing an empty env name (or "PREFIX_" if an env prefix is set). This means if a new exported field is added without a
mapstructuretag, the function silently passes an empty key tov.BindEnv(key), resulting in misbinding that could be difficult to debug.To prevent this and mirror the stricter checks in the env-doc generator, add validation:
func setDefaultConfig(v *viper.Viper) error { t := reflect.TypeOf(Config{}) for i := 0; i < t.NumField(); i++ { f := t.Field(i) if !f.IsExported() { continue } - key := f.Tag.Get("mapstructure") + key := strings.TrimSpace(f.Tag.Get("mapstructure")) + if key == "" { + return fmt.Errorf("config field %s missing mapstructure tag", f.Name) + } def := f.Tag.Get("envDefault") if def != "" { v.SetDefault(key, def) } - err := v.BindEnv(key) - if err != nil { + if err := v.BindEnv(key); err != nil { return fmt.Errorf("error binding env variable for key %s: %w", key, err) } } return nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/config/config.go(6 hunks)tools/gen-env-doc/main.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tools/gen-env-doc/main.go (1)
internal/config/config.go (1)
Config(27-58)
internal/config/config.go (2)
internal/core/domain/settings.go (4)
LnConnectionOpts(14-18)ConnectionType(7-7)LND_CONNECTION(11-11)CLN_CONNECTION(10-10)utils/url.go (1)
ValidateURL(25-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: integration tests
🔇 Additional comments (6)
internal/config/config.go (6)
27-58: Config struct tagging cleanly centralizes env defaults and docs.The
mapstructure,envDefault, andenvInfotags onConfiggive a single source of truth for both runtime config (via Viper) and env docs generation. The new LN-related fields andNoMacaroonsflag are documented with clear descriptions and examples, and unexported internals (lnConnectionOpts,unlocker,macaroonSvc) stay out of the env surface.
60-92: LoadConfig flow and initialization ordering look correct.Creating a fresh
viper.Viper, applyingsetDefaultConfig, then unmarshalling intoConfigfollowed byinitDb,deriveLnConfig, unlocker, and macaroon initialization results in a clear, linear startup path. Using methods onConfigkeeps the load logic cohesive and makes it easier to test in isolation.
157-165: Config.deriveLnConfig nicely encapsulates LN opts wiring.Delegating to the pure function and stashing the result in
c.lnConnectionOptskeeps the stateful part minimal and makes it easy to reusederiveLnConfigin tests.
171-173: Getter for LN connection options is a reasonable encapsulation step.Exposing
GetLnConnectionOpts()instead of the field allows you to keeplnConnectionOptsunexported and maintain flexibility for future changes (e.g., lazy derivation or caching tweaks) without breaking callers.
230-239:NoMacaroonstoggle integrates cleanly with macaroon service init.Guarding
macaroon.NewServicewithif !c.NoMacaroonsmakes the opt-out behavior explicit and keeps the initialization idempotent via the existingc.macaroonSvc != nilguard.
361-361:go:generatehook is a nice way to keep env docs in sync.Linking the config package to
../../tools/gen-env-doc/main.govia//go:generate(plus the CI check) is a good way to ensuredocs/environment.mdstays consistent withConfigchanges.
close #303
Summary by CodeRabbit
New Features
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.