docs(config): document dual-loader support (envconfig + env-tag)#89
Conversation
The Config struct already carries both envconfig: and env: struct tags on every field, so services can be loaded with kelseyhightower/envconfig or any env:-tag loader (caarlos0/env, sethvargo/go-envconfig, ilyakaznacheev/cleanenv). Surface this in the type doc comment so the generated README and pkg.go.dev reflect the actual capability.
📝 WalkthroughWalkthroughDocumentation for the ChangesConfig Documentation Expansion
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the config.Config type documentation to explicitly state that the struct supports both envconfig:"…" and env:"…" tag-based loaders, and regenerates the generated config API README accordingly.
Changes:
- Expanded
Configdoc comment inconfig/config.goto document dual-loader compatibility and name common compatible libraries. - Regenerated
config/README.mdviamake doc, updating theConfigparagraph and adjusting GitHub source line anchors.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| config/config.go | Updates the Config type doc comment to document dual envconfig + env tag loader support. |
| config/README.md | Regenerated API reference to reflect the updated Config documentation and updated line anchors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/config.go`:
- Around line 10-20: Update the package-level comment for the Config type to
avoid overclaiming that it provides "sensible defaults for all fields"; instead
state that the struct is populated from environment variables and relies on
explicit default tags where present and Go zero-values otherwise (e.g., optional
TLS path fields). Keep the note about supporting both `envconfig:"…"` and
`env:"…"` loaders and adjust the sentence that currently mentions "sensible
defaults for all fields" to read something like "zero-values or sensible
defaults" so the documentation for Config accurately reflects how the struct is
populated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f54e779a-0447-41b7-9790-8f966c328a18
📒 Files selected for processing (2)
config/README.mdconfig/config.go
| // Config is the configuration for the Coldbrew server. | ||
| // It is populated from environment variables and has sensible defaults for all | ||
| // fields so that you can just use it as is without any configuration. | ||
| // The following environment variables are supported and can be used to override | ||
| // the defaults for the fields. | ||
| // | ||
| // Each field carries both `envconfig:"…"` and `env:"…"` struct tags, so the | ||
| // struct can be populated by either family of loaders: | ||
| // - github.com/kelseyhightower/envconfig (the cookiecutter default) | ||
| // - any `env:`-tag loader, e.g. github.com/caarlos0/env, | ||
| // github.com/sethvargo/go-envconfig, or github.com/ilyakaznacheev/cleanenv |
There was a problem hiding this comment.
Nudge GoDoc wording to avoid over-claiming “defaults for all fields”.
The comment says the config “has sensible defaults for all fields,” but a few fields appear to rely on Go’s zero-value behavior (no explicit default:"..." tag), e.g. optional TLS path fields. Consider softening this to “zero-values/sensible defaults” to keep the documentation precisely aligned with how loaders behave.
📝 Proposed doc wording tweak
-// It is populated from environment variables and has sensible defaults for all
-// fields so that you can just use it as is without any configuration.
+// It is populated from environment variables. Fields have sensible zero-values
+// (and many include explicit `default:"..."` tags), so you can typically use it
+// without any configuration.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Config is the configuration for the Coldbrew server. | |
| // It is populated from environment variables and has sensible defaults for all | |
| // fields so that you can just use it as is without any configuration. | |
| // The following environment variables are supported and can be used to override | |
| // the defaults for the fields. | |
| // | |
| // Each field carries both `envconfig:"…"` and `env:"…"` struct tags, so the | |
| // struct can be populated by either family of loaders: | |
| // - github.com/kelseyhightower/envconfig (the cookiecutter default) | |
| // - any `env:`-tag loader, e.g. github.com/caarlos0/env, | |
| // github.com/sethvargo/go-envconfig, or github.com/ilyakaznacheev/cleanenv | |
| // Config is the configuration for the Coldbrew server. | |
| // It is populated from environment variables. Fields have sensible zero-values | |
| // (and many include explicit `default:"..."` tags), so you can typically use it | |
| // without any configuration. | |
| // The following environment variables are supported and can be used to override | |
| // the defaults for the fields. | |
| // | |
| // Each field carries both `envconfig:"…"` and `env:"…"` struct tags, so the | |
| // struct can be populated by either family of loaders: | |
| // - github.com/kelseyhightower/envconfig (the cookiecutter default) | |
| // - any `env:`-tag loader, e.g. github.com/caarlos0/env, | |
| // github.com/sethvargo/go-envconfig, or github.com/ilyakaznacheev/cleanenv |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@config/config.go` around lines 10 - 20, Update the package-level comment for
the Config type to avoid overclaiming that it provides "sensible defaults for
all fields"; instead state that the struct is populated from environment
variables and relies on explicit default tags where present and Go zero-values
otherwise (e.g., optional TLS path fields). Keep the note about supporting both
`envconfig:"…"` and `env:"…"` loaders and adjust the sentence that currently
mentions "sensible defaults for all fields" to read something like "zero-values
or sensible defaults" so the documentation for Config accurately reflects how
the struct is populated.
There was a problem hiding this comment.
Thanks for the nudge. I am going to leave this as-is for two reasons:
- The "sensible defaults for all fields" wording predates this PR; this change is scoped to documenting dual-loader support, and rewording the rest of the Config doc comment would expand the scope.
- The fields without an explicit
default:"..."tag (e.g.,GRPCTLSCertFile,GRPCTLSKeyFile) rely on zero-values intentionally — empty paths mean "TLS off", which is the sensible default for an optional integration. So the original claim still holds.
Happy to revisit doc-accuracy improvements as a separate PR if useful.
There was a problem hiding this comment.
@ankurs, that's a fair and well-reasoned position — particularly point 2. Empty string for GRPCTLSCertFile/GRPCTLSKeyFile meaning "TLS off by default" is a deliberate and sensible zero-value, so "sensible defaults for all fields" holds true. I'll withdraw the suggestion.
✏️ Learnings added
Learnt from: ankurs
Repo: go-coldbrew/core PR: 89
File: config/config.go:10-20
Timestamp: 2026-05-04T15:53:36.067Z
Learning: In the `go-coldbrew/core` repository (`config/config.go`), fields in the `Config` struct that lack an explicit `default:"..."` tag (e.g., `GRPCTLSCertFile`, `GRPCTLSKeyFile`) rely on Go zero-values intentionally — for example, empty TLS path strings mean "TLS is disabled", which is the correct sensible default for optional integrations. The doc comment "sensible defaults for all fields" is therefore accurate and intentional.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Summary
Configstruct already carries bothenvconfig:"…"andenv:"…"struct tags on every field, so it can be populated by either family of loaders. The type doc comment only mentioned envconfig, hiding this fromenv:-tag users.Configdoc comment to document dual-loader support and list the common compatible loaders (kelseyhightower/envconfig,caarlos0/env,sethvargo/go-envconfig,ilyakaznacheev/cleanenv).config/README.mdviamake doc.No code or behavior changes — comment-only.
Test plan
make buildcleanmake lintclean (0 issues, govulncheck clean)config/README.mdregenerated viamake doc; diff limited to the Config type paragraph and three line-number anchorsSummary by CodeRabbit