feat: integration of viper configuration management in api #311
Conversation
|
To generate Unit Tests for this PR, please click here. |
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 19593524 | Triggered | Generic Password | 3fb121e | helpers/config.dev.yaml | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
Note Reviews pausedUse the following commands to manage reviews:
WalkthroughCentralizes configuration using a nested AppConfig loaded via Viper from files and environment. Refactors types to structured configs, updates many modules to read from AppConfig instead of os.Getenv, initializes storage and migrations in config.Init, adds comprehensive config tests, updates dependencies (adds Viper), and tweaks a dev YAML default. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as Operator
participant Main as main.go
participant Cfg as config.Init
participant V as Viper
participant Store as storage.NewDB
participant Mig as storage.RunMigrations
participant HTTP as Router/Server
participant Feat as Feature Modules
User->>Main: Start API
Main->>Cfg: Init()
Cfg->>V: Select config file/paths + AutomaticEnv
V-->>Cfg: Load + Unmarshal
Cfg->>Cfg: Validate config
Cfg->>Store: Build DB config and connect
Store-->>Cfg: DB handle
Cfg->>Mig: Run migrations
Mig-->>Cfg: OK
Cfg-->>Main: AppConfig ready
Main->>HTTP: Setup routes (uses AppConfig)
Main->>HTTP: Listen on AppConfig.Server.Port
Feat-->>HTTP: Handlers read from AppConfig (Redis, SSH, Proxy, etc.)
note over Feat,HTTP: Modules stopped reading env<br/>and use centralized AppConfig
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
raghavyuva
force-pushed
the
feat/viper_integration
branch
from
7113af9 to
207b66f
Compare
raghavyuva
changed the title
|
@coderabbitai pause |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
api/go.mod (3)
3-3: Fix:godirective must not include a patch version
go 1.23.6is invalid syntax for thegodirective; it must be major.minor only. Keep the patch granularity only in thetoolchaindirective.Apply this diff:
-go 1.23.6 +go 1.23
1-1: ✏️ Alignapi/go.modModule Path with Repository LayoutThe Git remote indicates the repository is
github.com/raghavyuva/nixopus.git, notnixopus-api. As-is, consumers importing your API will look forgithub.com/raghavyuva/nixopus-api/..., which does not match this repo’s path. To prevent broken imports, update the module declaration to reflect theapi/subdirectory within thenixopusrepository.• File needing update:
api/go.mod(line 1)• Change the module path from:
-module github.com/raghavyuva/nixopus-api +module github.com/raghavyuva/nixopus/apiIf you do intend to publish the API as a separate module under
github.com/raghavyuva/nixopus-api, please ensure you’ve set up that repository and remote accordingly. Otherwise, apply the above change to maintain a consistent import path.
5-5: Action Required: Bump CI Go version to matchtoolchain go1.23.7Your GitHub Actions workflows currently install Go 1.22, which is older than the version declared in your go.mod
toolchaindirective. This mismatch will cause CI failures because the Go command enforces that it be at least the toolchain version specified. Please update allactions/setup-goinvocations to use Go 1.23.7 (or"1.23"to follow the latest patch) in:
- .github/workflows/test.yaml
- .github/workflows/format.yaml
- .github/workflows/dependabot.yml
Example diff for each workflow:
- uses: actions/setup-go@v5 with: - go-version: '1.22' + go-version: '1.23.7' check-latest: true cache: trueEnsure any other CI images, local dev containers, or buildpacks are also updated to Go ≥ 1.23.7 so that
gorecognizes and applies thetoolchaindirective.api/internal/features/update/service/update_service.go (1)
99-105: Update in-memory AppConfig and correct getCurrentVersion messagingThe
PerformUpdatemethod currently only exports the new version to the environment but never updatesconfig.AppConfig.App.Version, so any in-process calls togetCurrentVersionwill continue returning the old version. Additionally,getCurrentVersionstill refers to “.env file” in its comment and error message even though it now reads from the in-memory config.Please apply the following changes:
• In api/internal/features/update/service/update_service.go, method getCurrentVersion (around lines 97–105):
– Update the comment to reflect that the version comes fromAppConfig.
– Change the error message to indicate the version isn’t configured inAppConfig.• In the same file, in PerformUpdate (around the
os.Setenv("APP_VERSION", latestVersion)call at line 191):
– After fetchinglatestVersion, assign it toconfig.AppConfig.App.Version.
– (Optionally) still callos.Setenvfor backward compatibility, discarding its error.@@ -95,9 +95,9 @@ -// getCurrentVersion gets the current version from the .env file +// getCurrentVersion returns the current app version from the in-memory configuration func (s *UpdateService) getCurrentVersion() (string, error) { version := config.AppConfig.App.Version if version != "" { return version, nil } - - return "", fmt.Errorf("APP_VERSION not found in .env file") + return "", fmt.Errorf("app version not configured (AppConfig.App.Version)") } @@ -187,11 +187,16 @@ latestVersion, err := s.fetchLatestVersion() if err != nil { return fmt.Errorf("failed to fetch latest version: %w", err) } - err = os.Setenv("APP_VERSION", latestVersion) - if err != nil { - return fmt.Errorf("failed to set APP_VERSION: %w", err) - } + // Update the in-memory config so getCurrentVersion returns the new value immediately + config.AppConfig.App.Version = latestVersion + // Optional: still export to env for external consumers + _ = os.Setenv("APP_VERSION", latestVersion) s.logger.Log(logger.Info, "Update completed successfully", "") return nil }api/internal/features/github-connector/service/authenticated_url.go (1)
15-35: Avoid hardcoding host and embedding tokens directly in URLs (security + GHES support).
- The HTTPS branch ignores the original host and always forces github.com. This breaks GitHub Enterprise Server (GHES) and any non-standard host.
- Embedding the access token in the URL risks leakage via logs, analytics, crash reports, and shell history.
Refactor to preserve the original host and set credentials via url.UserPassword. For SSH remotes, parse the host instead of assuming github.com.
func (s *GithubConnectorService) CreateAuthenticatedRepoURL(repoURL, accessToken string) (string, error) { - if strings.HasPrefix(repoURL, "https://") { - parsedURL, err := url.Parse(repoURL) - if err != nil { - return "", fmt.Errorf("invalid repository URL: %w", err) - } - - return fmt.Sprintf("https://oauth2:%s@github.com%s", accessToken, parsedURL.Path), nil - } else if strings.HasPrefix(repoURL, "git@github.com") { - parts := strings.Split(strings.TrimPrefix(repoURL, "git@github.com:"), "/") - if len(parts) < 2 { - return "", fmt.Errorf("invalid SSH repository URL format") - } - - owner := parts[0] - repo := strings.TrimSuffix(parts[len(parts)-1], ".git") - - return fmt.Sprintf("https://oauth2:%s@github.com/%s/%s.git", accessToken, owner, repo), nil - } + if strings.HasPrefix(repoURL, "https://") { + u, err := url.Parse(repoURL) + if err != nil { + return "", fmt.Errorf("invalid repository URL: %w", err) + } + // Preserve original host and path; inject token as userinfo. + u.User = url.UserPassword("oauth2", accessToken) + return u.String(), nil + } else if strings.HasPrefix(repoURL, "git@") { + // Support generic "git@<host>:<owner>/<repo>(.git)?" format. + withoutPrefix := strings.TrimPrefix(repoURL, "git@") + hostAndPath := strings.SplitN(withoutPrefix, ":", 2) + if len(hostAndPath) != 2 { + return "", fmt.Errorf("invalid SSH repository URL format") + } + host := hostAndPath[0] + pathParts := strings.Split(hostAndPath[1], "/") + if len(pathParts) < 2 { + return "", fmt.Errorf("invalid SSH repository URL format") + } + owner := pathParts[0] + repo := strings.TrimSuffix(pathParts[len(pathParts)-1], ".git") + u := &url.URL{ + Scheme: "https", + Host: host, + Path: fmt.Sprintf("/%s/%s.git", owner, repo), + User: url.UserPassword("oauth2", accessToken), + } + return u.String(), nil + } return "", fmt.Errorf("unsupported repository URL format") }Follow-up:
- Prefer avoiding tokens in URLs altogether (e.g., use go-git with BasicAuth or GIT_ASKPASS) to eliminate accidental leakage. Want a patch for that?
api/internal/routes.go (1)
62-66: Fatal on missing .env will crash deployments after Viper integration. Make .env optional or remove.With centralized config loading, requiring .env is unnecessary and harmful in environments where .env isn’t present.
- err := godotenv.Load() - if err != nil { - log.Fatal("Error loading .env file") - } + if err := godotenv.Load(); err != nil { + // Optional: .env not present in many environments; rely on Viper/AppConfig. + log.Printf("No .env file loaded: %v (continuing with AppConfig)", err) + }Alternatively, remove this block entirely and ensure config.Init is called during process bootstrap.
api/internal/features/ssh/init.go (1)
148-194: Terminal(): avoid panic, close client, prefer golang.org/x/term.
- Panics in library code degrade server robustness.
- Client is never closed -> potential resource leak.
- ssh/terminal is deprecated; x/term should be used.
-func (s *SSH) Terminal() { - client, err := s.Connect() +func (s *SSH) Terminal() { + client, err := s.Connect() if err != nil { fmt.Print("Failed to connect to ssh") return } + defer client.Close() @@ - fileDescriptor := int(os.Stdin.Fd()) - if terminal.IsTerminal(fileDescriptor) { - originalState, err := terminal.MakeRaw(fileDescriptor) + fileDescriptor := int(os.Stdin.Fd()) + if terminal.IsTerminal(fileDescriptor) { + originalState, err := terminal.MakeRaw(fileDescriptor) if err != nil { - panic(err) + fmt.Printf("Failed to make terminal raw: %v\n", err) + return } defer terminal.Restore(fileDescriptor, originalState) @@ - termWidth, termHeight, err := terminal.GetSize(fileDescriptor) + termWidth, termHeight, err := terminal.GetSize(fileDescriptor) if err != nil { - panic(err) + fmt.Printf("Failed to get terminal size: %v\n", err) + return } @@ - err = session.RequestPty("xterm-256color", termHeight, termWidth, modes) - if err != nil { - panic(err) - } + if err = session.RequestPty("xterm-256color", termHeight, termWidth, modes); err != nil { + fmt.Printf("Failed to request PTY: %v\n", err) + return + } } @@ - err = session.Shell() - if err != nil { - return - } + if err = session.Shell(); err != nil { + fmt.Printf("Failed to start shell: %v\n", err) + return + } session.Wait() }If you’re open to it, I can also swap ssh/terminal for golang.org/x/term in a follow-up patch to eliminate deprecation.
api/internal/realtime/postgres_listener.go (1)
66-85: Don’t use select with default here; it prevents clean ctx cancellation and complicates flow.default always fires, so the ctx.Done branch is effectively unreachable. Let WaitForNotification block with ctx and exit on cancellation.
- go func() { - defer conn.Close(ctx) - for { - select { - case <-ctx.Done(): - return - default: - notification, err := conn.WaitForNotification(ctx) - if err != nil { - time.Sleep(5 * time.Second) - continue - } - c.notificationChan <- &PostgresNotification{ - Channel: notification.Channel, - Payload: notification.Payload, - } - } - } - }() + go func() { + defer conn.Close(ctx) + for { + notification, err := conn.WaitForNotification(ctx) + if err != nil { + // Exit cleanly on context cancellation + if ctx.Err() != nil { + return + } + time.Sleep(5 * time.Second) + continue + } + c.notificationChan <- &PostgresNotification{ + Channel: notification.Channel, + Payload: notification.Payload, + } + } + }()api/internal/config/config.go (1)
71-76: Default server port is applied too late and is effectively unreachable; set defaults in Viper and remove late post-validate fallback.You validate required fields before assigning the fallback port, so an empty port will fail validation and never reach the defaulting branch. Define defaults in initViper prior to ReadInConfig so Unmarshal sees them, and drop the redundant err check after migrations.
func initViper() { configName := getConfigFileName() viper.SetConfigName(configName) viper.SetConfigType("yaml") @@ viper.AutomaticEnv() setupEnvVarMappings() + + // Defaults must be set before reading config and unmarshalling + viper.SetDefault("server.port", "8080") + // Optional sensible DB defaults: + // viper.SetDefault("database.ssl_mode", "disable") + // viper.SetDefault("database.max_open_conn", 10) + // viper.SetDefault("database.max_idle_conn", 5) + // viper.SetDefault("database.debug", false) if err := viper.ReadInConfig(); err != nil { @@ } @@ - if AppConfig.Server.Port == "" { - AppConfig.Server.Port = "8080" - } - if err != nil { - log.Fatalf("Failed to initialize postgres client: %v", err) - } + // Port default is handled by Viper defaults above. The extra err check is redundant.Also applies to: 89-130
🧹 Nitpick comments (35)
api/go.mod (3)
26-26: New direct dep: Viper v1.20.1 — align downstream usageGood addition for centralized config. Two follow-ups to avoid subtle runtime surprises:
- Mapstructure v2: Viper 1.20 pulls
github.com/go-viper/mapstructure/v2. Ensure your config structs use v2-compatiblemapstructure:"..."tags consistently (especially for nested/embedded structs and time/duration fields).- ENV precedence and naming: If you rely on env overrides, make sure you explicitly set
AutomaticEnv(),SetEnvPrefix, and aSetEnvKeyReplacerto normalize dot/underscore/hyphen keys, so env overrides work predictably across platforms.If not already present in the PR code, consider adding a small config bootstrap with explicit precedence ordering: defaults < file < env < flags.
Would you like me to draft a minimal
config.Init()snippet and table-driven tests to lock in precedence and key mapping behavior?
36-36: Remove directgopkg.in/yaml.v3if unused directlyViper (and some transitive deps) already bring YAML v3 indirectly. Keeping it as a direct requirement when the repo doesn’t import it can cause unnecessary version pinning and potential resolution conflicts with other tools (you also have
github.com/oasdiff/yamlandyaml3transitives).Apply this diff if you don’t import
gopkg.in/yaml.v3in your code:- gopkg.in/yaml.v3 v3.0.1If you do import it, all good—keep it direct and add a brief comment explaining why it’s pinned (e.g., compatibility with a specific YAML feature).
63-63: New indirects pulled by Viper — confirm struct tags and decoding edge casesThe added indirects (
mapstructure/v2,pelletier/go-toml/v2,afero,cast,pflag,gotenv, etc.) are expected with Viper 1.20.x. Focus on:
- mapstructure v2 decoding of time.Duration, slices, and pointer fields (ensure
mapstructure:",squash"and custom decoders where needed),- consistent file type handling if you support multiple formats (YAML/TOML/ENV),
- no accidental reliance on
castbehavior for zero-values.If you have complex/nested config, I recommend adding round-trip tests: file → struct → re-marshal → compare.
Also applies to: 100-100, 106-113, 128-129
helpers/config.dev.yaml (2)
8-8: Avoid shipping meaningful secrets as defaults, even in dev."postgres" is a common real password in many setups. Prefer a clearly placeholder value (e.g., "dev-password") to reduce accidental reuse.
Possible tweak:
- PASSWORD: ${PASSWORD:-postgres} + PASSWORD: ${PASSWORD:-dev-password}If you adopt this, remember to mirror it in POSTGRES_PASSWORD as noted above.
52-52: Reminder: POSTGRES_HOST_AUTH_METHOD=trust must remain dev-only.Keeping trust in the dev profile is fine, but ensure it is never enabled in staging/production configs.
api/internal/features/deploy/service/deploy_service.go (1)
170-170: Use centralized mount path (good); add a minimal guard/fallback for robustness.repoPath now derives from config.AppConfig.Deployment.MountPath. If this field is unset due to misconfiguration, cleanup may silently target an empty/incorrect path. A small defensive check keeps behavior predictable in dev/test runs.
Apply this diff to guard the base path (while keeping the new config-first behavior):
- repoPath := filepath.Join(config.AppConfig.Deployment.MountPath, userID.String(), string(application.Environment), application.ID.String()) + base := config.AppConfig.Deployment.MountPath + if base == "" { + // Optional: preserve previous default, if any, or choose a safe local fallback + base = "./configs" + } + repoPath := filepath.Join(base, userID.String(), string(application.Environment), application.ID.String())If you’d prefer strictness, replace the fallback with an early error log and return.
api/main.go (1)
33-35: Consider an http.Server with sane timeouts and graceful shutdown.While orthogonal to this PR, moving away from http.ListenAndServe(nil) improves resilience for long-lived connections and slowloris-style issues.
Apply this diff and add an import for "time":
- log.Printf("Server starting on port %s", config.AppConfig.Server.Port) - log.Fatal(http.ListenAndServe(":"+config.AppConfig.Server.Port, nil)) + log.Printf("Server starting on port %s", config.AppConfig.Server.Port) + srv := &http.Server{ + Addr: ":" + config.AppConfig.Server.Port, + Handler: nil, // default mux already registered by router.Routes() + ReadHeaderTimeout: 10 * time.Second, + ReadTimeout: 30 * time.Second, + WriteTimeout: 30 * time.Second, + IdleTimeout: 60 * time.Second, + } + log.Fatal(srv.ListenAndServe())api/internal/middleware/cors.go (2)
21-28: Pulling allowed origin from config is good; add Vary and remove noisy stdout.
- Keep origin selection logic, but add Vary headers to ensure proper cache behavior per origin.
- Drop fmt.Println; use your logger if needed.
Apply this diff:
- fmt.Println("allowedOrigin", allowedOrigin) + // Consider logging via your logger if you need visibility. + // s.logger.Log(logger.Debug, "allowedOrigin", allowedOrigin) // example @@ - if originAllowed { + // Make CORS responses cache-aware per origin. + w.Header().Add("Vary", "Origin") + w.Header().Add("Vary", "Access-Control-Request-Method") + w.Header().Add("Vary", "Access-Control-Request-Headers") + if originAllowed { w.Header().Set("Access-Control-Allow-Origin", origin)
21-28: Optional: Support multiple configured origins.If you expect more than one non-localhost origin, consider adding CORS.AllowedOrigins []string to config and append defaults here.
Example sketch:
- allowedOrigin := config.AppConfig.CORS.AllowedOrigin + allowedOrigin := config.AppConfig.CORS.AllowedOrigin + cfgOrigins := config.AppConfig.CORS.AllowedOrigins // []string, optional @@ - allowedOrigins := []string{ - allowedOrigin, + allowedOrigins := append([]string{ + allowedOrigin, // keep backward compatibility "http://localhost:3000", "http://localhost:7443", - } + }, cfgOrigins...)api/internal/features/update/controller/update_controller.go (1)
38-42: Align dev-mode short-circuit and auth checks in PerformUpdate
- In
api/internal/features/update/controller/update_controller.go(lines 78–83), the dev-mode early return currently comes before theuser == nilauth check. This lets unauthenticated dev requests succeed. Swap the blocks so that you enforce authentication first, then short-circuit in development.- Introduce a helper in the config package—e.g.
to avoid repeating thefunc IsDev() bool { return AppConfig.App.Environment == "development" }"development"literal and reduce drift.- Verified that
config.Init()is invoked inapi/main.go(line 22) prior to wiring any HTTP handlers, and is also called in domain tests and fixtures—soAppConfigwill be initialized at startup.Suggested diff for PerformUpdate:
@@ PerformUpdate - // If the environment is development, we will not perform updates - if config.AppConfig.App.Environment == "development" { - return &types.UpdateResponse{ - Success: true, - Message: "Update completed successfully", - }, nil - } - - if user == nil { + if user == nil { return nil, fuego.HTTPError{Err: nil, Status: http.StatusUnauthorized} } + + // Short-circuit in development after auth + if config.IsDev() { + return &types.UpdateResponse{ + Success: true, + Message: "Update completed successfully", + }, nil + }api/internal/features/notification/helpers/email/email_helper.go (1)
144-146: Build URLs via net/url and validate CORS.AllowedOrigin to prevent malformed linksUsing string concatenation risks double slashes or missing scheme/host and makes invalid config hard to spot. Parse and join paths via net/url, and fail early on bad origin.
Apply these diffs:
@@ - "net/smtp" + "net/smtp" + "net/url" @@ - viewURL := config.AppConfig.CORS.AllowedOrigin - resetURL := fmt.Sprintf("%s/reset-password?token=%s", viewURL, token) + u, err := url.Parse(config.AppConfig.CORS.AllowedOrigin) + if err != nil || u.Scheme == "" || u.Host == "" { + return fmt.Errorf("invalid CORS.AllowedOrigin in config") + } + path, _ := url.JoinPath(u.String(), "reset-password") + resetURL := fmt.Sprintf("%s?token=%s", path, token) @@ - viewURL := config.AppConfig.CORS.AllowedOrigin - verifyURL := fmt.Sprintf("%s/verify-email?token=%s", viewURL, token) + u, err := url.Parse(config.AppConfig.CORS.AllowedOrigin) + if err != nil || u.Scheme == "" || u.Host == "" { + return fmt.Errorf("invalid CORS.AllowedOrigin in config") + } + path, _ := url.JoinPath(u.String(), "verify-email") + verifyURL := fmt.Sprintf("%s?token=%s", path, token)Also applies to: 178-180
api/cmd/fixtures/main.go (2)
49-55: Avoid shadowing the imported config package with a local variable nameThe variable config from pgx.ParseConfig shadows the imported package name config, which reduces readability and can surprise maintenance tools. Rename it.
Apply this diff:
- config, err := pgx.ParseConfig(dsn) - if err != nil { - log.Fatalf("Failed to parse database config: %v", err) - } - - sqldb := stdlib.OpenDB(*config) + pgxCfg, err := pgx.ParseConfig(dsn) + if err != nil { + log.Fatalf("Failed to parse database config: %v", err) + } + + sqldb := stdlib.OpenDB(*pgxCfg)
35-41: Minor: consider logging which DB target you’re connecting toA small startup log using the values from AppConfig.Database (without password) can help diagnose misconfigurations when running fixtures.
Example (optional):
@@ host := config.AppConfig.Database.Host port := config.AppConfig.Database.Port username := config.AppConfig.Database.Username password := config.AppConfig.Database.Password dbName := config.AppConfig.Database.Name sslMode := config.AppConfig.Database.SSLMode + log.Printf("Connecting to DB %s@%s:%s/%s (sslmode=%s)", username, host, port, dbName, sslMode)api/internal/features/domain/validation/validator.go (1)
117-121: LGTM: centralized dev-mode gateSwitching to config.AppConfig.App.Environment keeps things consistent. If this check appears in multiple places, consider a helper like config.IsDev() to avoid littering string literals and to support case-insensitive matches.
- development := config.AppConfig.App.Environment == "development" + development := strings.EqualFold(config.AppConfig.App.Environment, "development")api/internal/features/update/service/update_service.go (3)
54-58: Environment detection tied to a single docker context; prefer App.Environment or make the context configurableHard-coding "nixopus-staging" couples behavior to a specific context name and diverges from the broader App.Environment flag. Either:
- key off App.Environment (preferred), or
- keep using Docker.Context, but compare case-insensitively and propagate that exact context into startContainers.
Option A (prefer App.Environment):
- if config.AppConfig.Docker.Context == "nixopus-staging" { + if strings.EqualFold(config.AppConfig.App.Environment, "staging") { return Staging }Also align startContainers to use the actual configured context (see separate comment below).
282-288: Propagate configured Docker context into the start command instead of hard-codingIf someone renames the staging context, startContainers will drift from env detection. Use the actual configured context with sensible defaults.
func (s *UpdateService) startContainers(ssh *ssh.SSH, paths PathConfig) error { - var startCmd string - if s.env == Staging { - startCmd = fmt.Sprintf("cd %s && DOCKER_HOST=unix:///var/run/docker.sock DOCKER_CONTEXT=nixopus-staging docker compose -f %s up -d --build 2>&1", paths.SourceDir, paths.ComposeFile) - } else { - startCmd = fmt.Sprintf("cd %s && DOCKER_HOST=unix:///var/run/docker.sock DOCKER_CONTEXT=default docker compose -f %s up -d 2>&1", paths.SourceDir, paths.ComposeFile) - } + var startCmd string + ctx := config.AppConfig.Docker.Context + if ctx == "" { + ctx = "default" + } + base := fmt.Sprintf("cd %s && DOCKER_HOST=unix:///var/run/docker.sock DOCKER_CONTEXT=%s docker compose -f %s up -d", paths.SourceDir, ctx, paths.ComposeFile) + if s.env == Staging { + startCmd = base + " --build 2>&1" + } else { + startCmd = base + " 2>&1" + }
115-121: Use an HTTP client with a timeout for fetching versionhttp.Get uses the default client without a timeout. A small timeout reduces the chance of indefinite hangs.
- resp, err := http.Get(url) + client := &http.Client{Timeout: 10 * time.Second} + resp, err := client.Get(url)api/internal/features/github-connector/service/authenticated_url.go (1)
67-77: Differentiate “not exists” vs other SFTP errors when deciding shouldPull.Currently only err == nil toggles pull. If Stat fails due to permissions or network, we fall through to MkdirAll and mask the error.
- info, err := sftp.Stat(clonePath) - if err == nil && info.IsDir() { - shouldPull = true - } + info, err := sftp.Stat(clonePath) + if err == nil { + if info.IsDir() { + shouldPull = true + } else { + return "", false, fmt.Errorf("clone path exists but is not a directory") + } + } else if !strings.Contains(err.Error(), "no such file or directory") { + // sftp does not expose os.IsNotExist directly; match provider error. + return "", false, fmt.Errorf("sftp stat failed: %w", err) + }api/internal/routes.go (3)
50-56: Guard against empty Redis URL before creating the cache client.Fail early with a clearer message if redis.url is missing; parsing an empty URL yields a generic error.
- cache, err := cache.NewCache(config.AppConfig.Redis.URL) + redisURL := config.AppConfig.Redis.URL + if redisURL == "" { + log.Fatal("Redis URL is empty: set redis.url in configuration") + } + cache, err := cache.NewCache(redisURL)
67-95: Ensure a valid port and centralize address formatting.
- If Server.Port is empty or non-numeric, fuego.NewServer will fail at listen time with a generic error.
- Consider enforcing a default (e.g., 8080) at config load.
- PORT := config.AppConfig.Server.Port + PORT := config.AppConfig.Server.Port + if PORT == "" { + log.Fatal("Server port is empty: set server.port in configuration") + }Also consider removing godotenv use entirely here to keep config ownership within the config package.
123-131: Development bypass for Swagger: extract to a reusable predicate and avoid hard-coded string.Minor readability/consistency improvement: provide a helper like config.IsDev() to encapsulate environment checks.
api/internal/features/ssh/init.go (3)
42-47: Redundant uint casts on Port.Port is already a uint in the struct; remove unnecessary conversions.
- Port: uint(s.Port), + Port: s.Port,Also applies to: 97-103
76-85: Dead code: parsePort is now unused post-Viper migration.Remove the function and its strconv import to reduce noise.
-import ( - "fmt" - "os" - "strconv" +import ( + "fmt" + "os" "github.com/melbahja/goph" "github.com/raghavyuva/nixopus-api/internal/config" "golang.org/x/crypto/ssh" - "golang.org/x/crypto/ssh/terminal" + "golang.org/x/crypto/ssh/terminal" ) @@ -func parsePort(port string) uint64 { - if port == "" { - return 22 - } - p, err := strconv.ParseUint(port, 10, 32) - if err != nil { - return 22 - } - return p -} +// parsePort removed: Port now comes from config as uint.
66-73: Don’t print raw errors; use structured logs and avoid leaking secrets.Printing “private key connection failed: …” to stdout can leak sensitive paths/details. Prefer a logger with levels and redact sensitive info.
api/internal/features/deploy/proxy/caddy.go (3)
52-57: Verify upstream host origin and handle empty SSH host.
- If ssh.host is empty, the Dial becomes “:port” and Caddy will reject the config.
- In containerized deployments, you might need an internal host/IP different from the public SSH host. Consider introducing a dedicated proxy.upstream_host setting.
- Dial: config.AppConfig.SSH.Host + ":" + c.Port, + Dial: config.AppConfig.SSH.Host + ":" + c.Port,Action items:
- Assert config.AppConfig.SSH.Host != "" before composing routes.
- Optionally add Proxy.UpstreamHost (or Proxy.ReverseProxyHost) to decouple reverse-proxy target from SSH host.
75-98: Assume “nixopus” server exists; add a defensive check.If the Caddy config doesn’t have servers["nixopus"], this will panic or silently create a zero-value server depending on map presence. Prefer a presence check.
- // Replace the route for our domain - server := config.Apps.HTTP.Servers["nixopus"] + // Replace the route for our domain + server, ok := config.Apps.HTTP.Servers["nixopus"] + if !ok { + return fmt.Errorf(`caddy server "nixopus" not found in current config`) + }
136-153: Avoid shadowing the imported config package with local identifiers.GetConfig and UpdateConfig use a local variable/parameter named config which hides the imported package, harming readability.
-func (c *Caddy) GetConfig() (CaddyConfig, error) { +func (c *Caddy) GetConfig() (CaddyConfig, error) { @@ - var config CaddyConfig - if err := json.NewDecoder(resp.Body).Decode(&config); err != nil { - return CaddyConfig{}, fmt.Errorf("failed to decode config: %w", err) - } - return config, nil + var cfg CaddyConfig + if err := json.NewDecoder(resp.Body).Decode(&cfg); err != nil { + return CaddyConfig{}, fmt.Errorf("failed to decode config: %w", err) + } + return cfg, nil } @@ -func (c *Caddy) UpdateConfig(config CaddyConfig) error { - jsonData, err := json.Marshal(config) +func (c *Caddy) UpdateConfig(cfg CaddyConfig) error { + jsonData, err := json.Marshal(cfg)api/internal/config/config_test.go (4)
98-107: Stabilize tests: isolate from accidental local config files and prefer t.Setenv.initViper searches several on-disk paths. If a developer happens to have a config file there, your “partial” env tests can read unexpected values. Also, deferring Unsetenv in a loop is verbose. Use t.Setenv and force Viper to search a non-existent directory to keep tests deterministic.
Apply this diff inside each subtest before calling initViper:
for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - for key, value := range tt.envVars { - os.Setenv(key, value) - defer os.Unsetenv(key) - } + for key, value := range tt.envVars { + t.Setenv(key, value) + } + // Ensure no real config file is picked up from default paths + t.Setenv("NIXOPUS_CONFIG_PATH", t.TempDir()+"/missing") viper.Reset() initViper()
146-157: Replace os.Setenv/Unsetenv with t.Setenv for cleaner lifecycle.t.Setenv automatically restores the environment after the test and avoids manual defers.
t.Run("Environment variables override config file", func(t *testing.T) { - os.Setenv("PORT", "9999") - defer os.Unsetenv("PORT") + t.Setenv("PORT", "9999") viper.Reset() initViper()
342-351: Prefer t.Setenv for ENV to avoid leaking state across subtests.This also lets you drop the conditional defer.
t.Run(tt.name, func(t *testing.T) { - if tt.envValue != "" { - os.Setenv("ENV", tt.envValue) - defer os.Unsetenv("ENV") - } + if tt.envValue != "" { + t.Setenv("ENV", tt.envValue) + } configName := getConfigFileName() assert.Equal(t, tt.expectedConfig, configName, tt.description) })
79-95: Table-driven asserts currently compare many zero-values; consider asserting only keys provided in each case.When a config file appears, zero-value expectations (e.g., empty usernames) can become brittle. Asserting only the fields explicitly set in the test case makes the tests resilient to the presence of defaults.
If you want, I can push a helper like assertMatchesProvided(t, expected, actual) that only checks non-zero fields on expected.
Also applies to: 113-130
api/internal/config/config.go (3)
48-59: Avoid snake_case and package shadowing to improve readability.
- storage_config should be storageConfig (Go style).
- The local variable named storage shadows the imported storage package, which is confusing and can cause surprises. Name it storeSvc or appStore instead.
- storage_config := storage.Config{ + storageConfig := storage.Config{ Host: AppConfig.Database.Host, Port: AppConfig.Database.Port, Username: AppConfig.Database.Username, Password: AppConfig.Database.Password, DBName: AppConfig.Database.Name, SSLMode: AppConfig.Database.SSLMode, MaxOpenConn: AppConfig.Database.MaxOpenConn, Debug: AppConfig.Database.Debug, MaxIdleConn: AppConfig.Database.MaxIdleConn, MigrationsPath: "./migrations", } -store, err := storage.NewDB(&storage_config) +store, err := storage.NewDB(&storageConfig) @@ -err = storage.RunMigrations(store, storage_config.MigrationsPath) +err = storage.RunMigrations(store, storageConfig.MigrationsPath) @@ -storage := storage.NewStore(store) +storeSvc := storage.NewStore(store) -err = storage.Init(context.Background()) +err = storeSvc.Init(context.Background()) @@ -return storage +return storeSvcAlso applies to: 61-69, 78-87
190-241: Don’t shadow the errors package and produce a cleaner message.Using a local slice named errors shadows the imported errors package (even if only within this function) and the fmt %v of a slice prints with brackets. Use errs and join.
-func validateConfig(config types.Config) error { - var errors []string +func validateConfig(config types.Config) error { + var errs []string if config.Server.Port == "" { - errors = append(errors, "server port is required") + errs = append(errs, "server port is required") } if config.Database.Host == "" { - errors = append(errors, "database host is required") + errs = append(errs, "database host is required") } if config.Database.Port == "" { - errors = append(errors, "database port is required") + errs = append(errs, "database port is required") } if config.Database.Username == "" { - errors = append(errors, "database username is required") + errs = append(errs, "database username is required") } if config.Database.Password == "" { - errors = append(errors, "database password is required") + errs = append(errs, "database password is required") } if config.Database.Name == "" { - errors = append(errors, "database name is required") + errs = append(errs, "database name is required") } if config.Redis.URL == "" { - errors = append(errors, "redis URL is required") + errs = append(errs, "redis URL is required") } if config.SSH.Host == "" { - errors = append(errors, "SSH host is required") + errs = append(errs, "SSH host is required") } if config.SSH.User == "" { - errors = append(errors, "SSH user is required") + errs = append(errs, "SSH user is required") } if config.Deployment.MountPath == "" { - errors = append(errors, "deployment mount path is required") + errs = append(errs, "deployment mount path is required") } if config.Proxy.CaddyEndpoint == "" { - errors = append(errors, "proxy caddy endpoint is required") + errs = append(errs, "proxy caddy endpoint is required") } if config.CORS.AllowedOrigin == "" { - errors = append(errors, "CORS allowed origin is required") + errs = append(errs, "CORS allowed origin is required") } - if len(errors) > 0 { - return fmt.Errorf("configuration validation failed: %v", errors) + if len(errs) > 0 { + return fmt.Errorf("configuration validation failed: %s", strings.Join(errs, "; ")) } return nil }
94-114: Minor: deduplicate AddConfigPath on repeated calls or guard initViper from being called twice.Repeated calls append the same paths multiple times. Not harmful, but you can defensively track whether paths were already added to keep the internal state clean (especially in tests).
Happy to add a once.Do guard or a small deduper if you want it.
Also applies to: 115-130
api/internal/types/types.go (1)
19-29: Optional: avoid duplicated validation logic.You have validate tags here but enforce required fields via custom validateConfig. Consider either a) removing the validate tags, or b) adopting a validator (e.g., go-playground/validator) to reflect these tags and reduce manual checks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
api/go.sumis excluded by!**/*.sum
📒 Files selected for processing (19)
api/cmd/fixtures/main.go(2 hunks)api/go.mod(6 hunks)api/internal/config/config.go(3 hunks)api/internal/config/config_test.go(1 hunks)api/internal/features/deploy/proxy/caddy.go(2 hunks)api/internal/features/deploy/service/deploy_service.go(2 hunks)api/internal/features/domain/tests/update_domain_test.go(3 hunks)api/internal/features/domain/validation/validator.go(2 hunks)api/internal/features/github-connector/service/authenticated_url.go(2 hunks)api/internal/features/notification/helpers/email/email_helper.go(3 hunks)api/internal/features/ssh/init.go(2 hunks)api/internal/features/update/controller/update_controller.go(3 hunks)api/internal/features/update/service/update_service.go(3 hunks)api/internal/middleware/cors.go(2 hunks)api/internal/realtime/postgres_listener.go(2 hunks)api/internal/routes.go(4 hunks)api/internal/types/types.go(1 hunks)api/main.go(1 hunks)helpers/config.dev.yaml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (17)
api/main.go (2)
api/internal/config/config.go (1)
AppConfig(18-18)api/internal/types/types.go (1)
AppConfig(62-66)
api/internal/features/deploy/service/deploy_service.go (1)
api/internal/config/config.go (1)
AppConfig(18-18)
api/internal/features/domain/validation/validator.go (4)
api/internal/config/config.go (1)
AppConfig(18-18)api/internal/types/types.go (1)
AppConfig(62-66)api/internal/storage/store.go (1)
App(17-21)api/internal/features/ssh/init.go (1)
SSH(14-22)
api/internal/features/ssh/init.go (2)
api/internal/config/config.go (1)
AppConfig(18-18)api/internal/types/types.go (1)
AppConfig(62-66)
api/internal/middleware/cors.go (2)
api/internal/config/config.go (1)
AppConfig(18-18)api/internal/types/types.go (1)
AppConfig(62-66)
api/internal/features/domain/tests/update_domain_test.go (3)
api/internal/config/config.go (2)
Init(24-87)AppConfig(18-18)api/internal/types/types.go (1)
AppConfig(62-66)api/internal/features/ssh/init.go (1)
SSH(14-22)
api/internal/features/github-connector/service/authenticated_url.go (2)
api/internal/config/config.go (1)
AppConfig(18-18)api/internal/types/types.go (1)
AppConfig(62-66)
api/internal/features/notification/helpers/email/email_helper.go (2)
api/internal/config/config.go (1)
AppConfig(18-18)api/internal/types/types.go (1)
AppConfig(62-66)
api/internal/features/update/service/update_service.go (3)
api/internal/config/config.go (1)
AppConfig(18-18)api/internal/types/types.go (1)
AppConfig(62-66)api/internal/storage/store.go (1)
App(17-21)
api/cmd/fixtures/main.go (2)
api/internal/config/config.go (2)
Init(24-87)AppConfig(18-18)api/internal/types/types.go (1)
AppConfig(62-66)
api/internal/features/update/controller/update_controller.go (2)
api/internal/config/config.go (1)
AppConfig(18-18)api/internal/types/types.go (1)
AppConfig(62-66)
api/internal/config/config_test.go (2)
api/internal/types/types.go (9)
Config(3-13)ServerConfig(15-17)DatabaseConfig(19-29)RedisConfig(31-33)SSHConfig(35-42)DeploymentConfig(44-46)ProxyConfig(54-56)CORSConfig(58-60)AppConfig(62-66)api/internal/config/config.go (1)
AppConfig(18-18)
api/internal/types/types.go (5)
api/internal/features/ssh/init.go (1)
SSH(14-22)api/internal/storage/store.go (1)
App(17-21)api/internal/config/config.go (1)
AppConfig(18-18)api/internal/features/logger/init.go (1)
Environment(8-8)api/internal/features/update/service/update_service.go (1)
Environment(22-22)
api/internal/routes.go (3)
api/internal/cache/cache.go (1)
NewCache(37-45)api/internal/config/config.go (1)
AppConfig(18-18)api/internal/types/types.go (1)
AppConfig(62-66)
api/internal/features/deploy/proxy/caddy.go (3)
api/internal/features/deploy/proxy/types.go (2)
FileServerType(19-19)Caddy(9-17)api/internal/config/config.go (1)
AppConfig(18-18)api/internal/features/ssh/init.go (1)
SSH(14-22)
api/internal/config/config.go (3)
api/internal/types/types.go (2)
Config(3-13)AppConfig(62-66)api/internal/storage/config.go (1)
Config(14-25)api/internal/features/ssh/init.go (1)
SSH(14-22)
api/internal/realtime/postgres_listener.go (2)
api/internal/config/config.go (1)
AppConfig(18-18)api/internal/types/types.go (2)
AppConfig(62-66)Config(3-13)
🔇 Additional comments (16)
api/go.mod (3)
22-22: sftp patch upgrade looks safeBumping
github.com/pkg/sftpto v1.13.7 is a patch-level change; no breaking API expected. Good to take.
53-53: fsnotify upgrade: re-test live-reload watchers
github.com/fsnotify/fsnotify v1.8.0includes platform fixes. If you depend on Viper’s live-reload (WatchConfig), re-test:
- watcher lifecycle (close on shutdown to avoid goroutine leaks),
- behavior on symlinked config files,
- event coalescing/debounce logic to prevent duplicate reloads.
22-22: No OSV vulnerabilities detected for updated dependenciesI ran the OSV check against the added/updated Go modules (
github.com/pkg/sftp@v1.13.7,github.com/spf13/viper@v1.20.1,gopkg.in/yaml.v3@v3.0.1,github.com/fsnotify/fsnotify@v1.8.0) and found no known vulnerabilities. No further action is needed before merging.helpers/config.dev.yaml (1)
12-12: Mapping Verified:MOUNT_PATHcorrectly bound toAppConfig.Deployment.MountPathI’ve confirmed that the environment variable
MOUNT_PATHis properly mapped through Viper into the deployment configuration:
- In
api/internal/config/config.go, you bind the env var to the nested key:viper.BindEnv("deployment.mount_path", "MOUNT_PATH")- In
api/internal/types/types.go, theDeploymentConfigstruct defines the field with the matching mapstructure tag:type DeploymentConfig struct { MountPath string `mapstructure:"mount_path" validate:"required"` }- The existing tests in
api/internal/config/config_test.goexercise both YAML and env var mappings, asserting thatconfig.Deployment.MountPathpicks up theMOUNT_PATHvalue correctly.No further changes are needed here.
api/internal/features/deploy/service/deploy_service.go (2)
11-11: LGTM: Imports updated to use centralized config.Switching from os.Getenv to config.AppConfig is consistent with the Viper migration and improves cohesion.
170-170: Deployment.MountPath field and Viper binding verified.Both requirements are satisfied:
- The
types.Configstruct includes a nestedDeploymentConfigwith aMountPath stringfield taggedmapstructure:"mount_path"(api/internal/types/types.go:44–46).- The configuration setup calls
viper.BindEnv("deployment.mount_path", "MOUNT_PATH")(api/internal/config/config.go:171).No further changes are needed.
api/main.go (2)
33-34: LGTM: Server port now sourced from AppConfig.Server.Port.Matches the nested config approach and the initializer’s defaulting of the port.
33-34: The scripts will verify that:
- A default for
server.portis set viaviper.SetDefault("server.port", …).- All defaults in
config.goto see if the port default appears elsewhere.- the
AppConfigstruct definition to confirmServer.Portexists and usesmapstructure:"port".- how Viper loads the config file and whether the key formats align.
Once results are in, we’ll confirm that
server.portis correctly bound and defaulted, or flag any discrepancies.api/internal/features/domain/tests/update_domain_test.go (1)
91-95: LGTM: Host derivation uses config with sane fallback.Reading from config.AppConfig.SSH.Host and falling back to os.Hostname() is robust and keeps the test portable.
api/internal/features/update/controller/update_controller.go (1)
7-7: LGTM: switched to centralized config sourceImporting internal/config aligns with the PR’s Viper-driven config. No issues here.
api/internal/features/notification/helpers/email/email_helper.go (1)
15-15: LGTM: unified config importMoving to config.AppConfig keeps the source of truth consistent. No issues.
api/internal/features/domain/validation/validator.go (1)
122-129: Validation logic for server host is correctly sourcing SSH.Host
- Confirmed that in
api/internal/config/config.go,setupEnvVarMappings()binds the Viper keyssh.hostto theSSH_HOSTenvironment variable, soconfig.AppConfig.SSH.HostreflectsSSH_HOST.- The existing fallback to
os.Hostname()only applies whenSSH_HOSTis not set or is empty.- No code changes are needed here; just ensure that in each non-development environment you set
SSH_HOSTto the public hostname that your managed domains resolve to, avoiding any unintended false negatives.api/internal/features/ssh/init.go (1)
26-32: LGTM: NewSSH now cleanly sources values from AppConfig.This removes env parsing and aligns with centralized configuration. Nice.
api/internal/features/deploy/proxy/caddy.go (1)
15-20: LGTM: centralizing Caddy endpoint in AppConfig with a sane default.Good default preservation and removal of os.Getenv usage.
api/internal/realtime/postgres_listener.go (1)
25-30: LGTM: Listener now uses centralized AppConfig.Good move to rely on the same source of truth as the rest of the app.
api/internal/types/types.go (1)
3-13: Struct layout and mapstructure tags look consistent with Viper bindings.The nested Config mirrors the env bindings in config.setupEnvVarMappings. SSH.Port as uint aligns with the SSH feature structs.
Also applies to: 15-66
✅ Actions performedReviews paused. |
1 participant
Summary by CodeRabbit
New Features
Refactor
Tests
Chores