[deps] add core library for koanf support
#177
Conversation
WalkthroughBumps Go version and dependencies in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App
participant Env as Environment
participant FS as FileSystem
participant Cfg as core/config
participant Log as Logger
App->>Env: read CONFIG_PATH
Env-->>App: value or nil
App->>App: localConfigPath = value or "config.yml"
App->>Cfg: Load(&defaultConfig, WithLocalYAML(localConfigPath))
Cfg->>FS: attempt read localConfigPath
alt file exists
FS-->>Cfg: bytes
Cfg-->>App: populated config
App->>Log: Info "Loaded config"
else file missing / read error
FS-->>Cfg: error
Cfg-->>App: error
App->>Log: Error "config load failed"
end
App-->>App: return defaultConfig (possibly merged)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing Touches
🧪 Generate unit tests
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/config/module.go (2)
91-93: Bug: defaulting API path is guarded by the wrong condition.This sets a default path when Host is empty; should check Path.
- if cfg.HTTP.API.Host == "" { - cfg.HTTP.API.Path = "/api" - } + if cfg.HTTP.API.Path == "" { + cfg.HTTP.API.Path = "/api" + }
101-103: Harden host normalization (strip any path/query).If a value like “example.com/api” is supplied, we should keep only “example.com[:port]”.
- // Guard against misconfigured scheme in host (accept "host[:port]" only) - cfg.HTTP.API.Host = strings.TrimPrefix(strings.TrimPrefix(cfg.HTTP.API.Host, "https://"), "http://") + // Guard against misconfigured scheme in host (accept "host[:port]" only) + cfg.HTTP.API.Host = strings.TrimPrefix(strings.TrimPrefix(cfg.HTTP.API.Host, "https://"), "http://") + if i := strings.Index(cfg.HTTP.API.Host, "/"); i >= 0 { + cfg.HTTP.API.Host = cfg.HTTP.API.Host[:i] + } + cfg.HTTP.API.Host = strings.TrimSuffix(cfg.HTTP.API.Host, "/")
🧹 Nitpick comments (6)
go.mod (1)
8-8: Pin core to a stable semver tag (v1.0.0)
Replace the pseudo-version with the latest non-pre-release tag (v1.0.0) in go.mod for reproducible builds.internal/config/config.go (2)
41-53: Add validation tags for DB settings.Guard common misconfigs at startup (e.g., invalid port, negative pool sizes).
Example:
- Port int `koanf:"port"` // database port + Port int `koanf:"port" validate:"min=1,max=65535"` // database port - MaxOpenConns int `koanf:"max_open_conns"` // max open connections + MaxOpenConns int `koanf:"max_open_conns" validate:"min=0"` // max open connections - MaxIdleConns int `koanf:"max_idle_conns"` // max idle connections + MaxIdleConns int `koanf:"max_idle_conns" validate:"min=0"` // max idle connections
55-59: Enforce minimums on FCM timings.To reflect “>= 5s” and avoid footguns, enforce via validation.
Example:
- DebounceSeconds uint16 `koanf:"debounce_seconds"` // push notification debounce (>= 5s) + DebounceSeconds uint16 `koanf:"debounce_seconds" validate:"min=5"` // push notification debounce (>= 5s)internal/config/module.go (3)
26-29: Avoid double getenv; minor clarity nit.- if os.Getenv("CONFIG_PATH") != "" { - localConfigPath = os.Getenv("CONFIG_PATH") - } + if v := os.Getenv("CONFIG_PATH"); v != "" { + localConfigPath = v + }
69-76: Optional: clamp debounce to documented minimum.Defensive defaulting avoids overly aggressive push loops.
- Debounce: time.Duration(cfg.FCM.DebounceSeconds) * time.Second, + Debounce: func() time.Duration { + d := time.Duration(cfg.FCM.DebounceSeconds) * time.Second + if d > 0 && d < 5*time.Second { + return 5 * time.Second + } + return d + }(),
121-126: Consider config hot-reload (optional).If core/config supports watch providers, wiring WithWatch(...) would let you live-reload local YAML in dev.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (3)
go.mod(4 hunks)internal/config/config.go(1 hunks)internal/config/module.go(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
internal/config/module.go (3)
internal/sms-gateway/modules/messages/service.go (1)
config(48-61)internal/sms-gateway/handlers/root.go (1)
config(11-16)internal/sms-gateway/modules/push/service.go (1)
config(36-46)
internal/config/config.go (1)
internal/sms-gateway/modules/messages/service.go (1)
config(48-61)
⏰ 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: Analyze (go)
🔇 Additional comments (4)
go.mod (1)
70-77: LGTM: koanf-related indirects align with the config migration.The koanf providers/parsers set matches the new loading path.
internal/config/config.go (1)
10-17: Struct tag migration to koanf looks consistent.Top-level sections (gateway, http, database, fcm, tasks, sse) map cleanly to nested keys.
internal/config/module.go (2)
41-47: LGTM: long WriteTimeout for SSE.
21-39: Verify config.Load precedence and legacy env names. Confirm that the loader applies ENV > file > defaults and that any deprecated CONFIG_* environment variables are still supported or documented.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
build/package/Dockerfile (6)
2-2: Go 1.24 base: pin for reproducibility; tag existsThe 1.24 line is good and published. Consider pinning to a patch and (optionally) Alpine minor or digest to avoid surprise rebuilds.
Refs: Go 1.24 release timeline and current patch train; 1.24-alpine tag is published. (tip.golang.org, docker.aityp.com)
Example (keeps it configurable and cache-friendly):
-FROM golang:1.24-alpine AS build +ARG GO_VERSION=1.24.7 +ARG ALPINE_VERSION=3.22 +FROM golang:${GO_VERSION}-alpine${ALPINE_VERSION:+${ALPINE_VERSION}} AS build
9-10: Avoid @latest for build toolsInstalling swag with @latest can break builds on upstream releases. Pin a known-good version.
-RUN go install github.com/swaggo/swag/cmd/swag@latest +ARG SWAG_VERSION=v1.16.3 +RUN go install github.com/swaggo/swag/cmd/swag@${SWAG_VERSION}
22-22: Version stamping twice? Verify sed vs. ldflagsYou already inject version via -X ldflags below; the sed replacement may be redundant or brittle. If main.go no longer needs the {APP_VERSION} placeholder, drop this step.
-RUN sed -i "s/{APP_VERSION}/${APP_VERSION}/g" ./cmd/${APP}/main.go +# (Optional) Remove if ldflags cover version stamping fully. +# RUN sed -i "s/{APP_VERSION}/${APP_VERSION}/g" ./cmd/${APP}/main.go
32-32: Pin final-stage Alpinealpine:3 floats across minors. Pin to a minor (or digest) to keep runtime stable.
-FROM alpine:3 AS prod +ARG ALPINE_VERSION=3.22 +FROM alpine:${ALPINE_VERSION} AS prod
39-41: Ensure entrypoint is executable by non-rootSince you switch to a non-root user later, confirm /docker-entrypoint.sh has +x and a proper shebang, or add an explicit chmod.
COPY scripts/docker-entrypoint.sh /docker-entrypoint.sh +RUN chmod +x /docker-entrypoint.sh
46-48: Create and use a dedicated non-root app user
Although alpine:3 includes a ‘guest’ user by default (see /etc/passwd: guest:x:405:100:guest… (gitlab.alpinelinux.org)), relying on it can be unclear—explicitly add and use a project-specific user for clarity and least privilege.-USER guest +# Create and use a dedicated app user +RUN adduser -D -g '' app && chown -R app:app /app +USER appbuild/package/Dockerfile.dev (3)
1-1: Go 1.24 base: pin versions for stabilitySame note as prod: pin Go patch and optionally Alpine minor/digest to avoid drift.
Refs: Go 1.24 series and availability of 1.24-alpine. (tip.golang.org, docker.aityp.com)
-FROM golang:1.24-alpine +ARG GO_VERSION=1.24.7 +ARG ALPINE_VERSION=3.22 +FROM golang:${GO_VERSION}-alpine${ALPINE_VERSION:+${ALPINE_VERSION}}
3-5: Drop redundant env and reconsider GOOS/CGO defaults
- GO111MODULE has been on by default since Go 1.16.
- GOOS=linux inside a Linux container is redundant and may affect cross-builds.
- CGO_ENABLED=0 can hinder libs/tools that expect cgo (ok if intentional).
-ENV GO111MODULE="on" -ENV GOOS="linux" -ENV CGO_ENABLED=0 +# ENV GO111MODULE is on by default since Go 1.16 +# ENV GOOS defaults to linux inside this image +# Enable/disable cgo per task; keep unset here unless strictly required +# ENV CGO_ENABLED=0
9-10: Pin dev toolsPin air and delve to known-good versions to avoid dev container churn when “latest” moves.
-RUN go install github.com/cosmtrek/air@latest && \ - go install github.com/go-delve/delve/cmd/dlv@latest +ARG AIR_VERSION=v1.52.3 +ARG DELVE_VERSION=v1.22.1 +RUN go install github.com/cosmtrek/air@${AIR_VERSION} && \ + go install github.com/go-delve/delve/cmd/dlv@${DELVE_VERSION}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
build/package/Dockerfile(1 hunks)build/package/Dockerfile.dev(1 hunks)
⏰ 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). (3)
- GitHub Check: Build / Docker image (linux/amd64)
- GitHub Check: Build / Docker image (linux/arm64)
- GitHub Check: Analyze (go)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
go.mod (1)
3-3: Avoid version skew: pin Go 1.24.x in CI and Docker too.Same ask as previously noted; ensure workflows and Dockerfiles use Go 1.24.x to match go.mod.
Run:
#!/bin/bash # Check GitHub Actions pins fd -a .github/workflows -t f | xargs -I{} rg -nH -C2 'go-version\s*:\s*' {} # Check Dockerfiles for golang base image versions find . -type f -name 'Dockerfile*' -exec grep -H -n 'FROM golang:' {} +
🧹 Nitpick comments (1)
go.mod (1)
8-8: Prefer a tagged release for core, if available.You’re using a pseudo-version. If the core repo has a tag for this commit, switch to the tag for clearer provenance and easier upgrades. Otherwise fine to keep as-is.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (2)
go.mod(4 hunks)internal/config/module.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/config/module.go
⏰ 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: Analyze (go)
🔇 Additional comments (3)
go.mod (3)
3-3: LGTM: bump to Go 1.24.1.go directive looks good.
70-77: Koanf stack looks consistent with the config migration.maps, parsers (yaml,dotenv), providers (env,file,s3) and v2 core align with the switch to core/config.
13-23: Sanity-check bumped validator, fx, and zap APIs
Grep across the codebase shows numerous import and usage locations for github.com/go-playground/validator/v10, go.uber.org/fx, and go.uber.org/zap—please compile and manually verify all critical call-sites (validator.New/Validate.Struct*/StructCtx/VarCtx, fx.Module/Provide/Invoke/WithLogger, zap field helpers like Any/String/Error and context-aware logger APIs) to catch any subtle breaking changes.
|
This PR is stale because it has been open for 7 days with no activity. |
Summary by CodeRabbit
New Features
Refactor
Chores