Skip to content

Conversation

@tolgaozen
Copy link
Member

@tolgaozen tolgaozen commented Nov 15, 2025

…n YAML files

Summary by CodeRabbit

  • New Features

    • Support for separate reader and writer database URIs to enable dedicated read/write database configurations.
  • Improvements

    • Standardized database configuration names (e.g., max_connections, min_connections, max_connection_lifetime_jitter) with backward-compatible aliases and updated documentation.
    • CLI flags and environment variables updated to match new naming.
    • Release version updated to v1.5.2.

@coderabbitai
Copy link

coderabbitai bot commented Nov 15, 2025

Walkthrough

This PR bumps the project version from v1.5.1 to v1.5.2 across API docs and proto metadata, and renames database connection-pool configuration and API/CLI options: max_connsmax_connections, min_connsmin_connections, and max_conn_lifetime_jittermax_connection_lifetime_jitter across code, docs, configs, flags, and tests.

Changes

Cohort / File(s) Summary
Version Bump
docs/api-reference/apidocs.swagger.json, docs/api-reference/openapiv2/apidocs.swagger.json, proto/base/v1/openapi.proto, internal/info.go
Updated API/OpenAPI/Swagger metadata and internal Version constant from v1.5.1v1.5.2.
Configuration Documentation & Examples
docs/setting-up/configuration.mdx, example.config.yaml, docs/performance-test/values.yaml
Renamed DB config keys (max_connsmax_connections, min_connsmin_connections, max_conn_lifetime_jittermax_connection_lifetime_jitter), added writer/reader URI entries, updated ENV mappings and deprecation notes.
Config Struct Definition
internal/config/config.go
Database struct field renames and mapstructure tag updates: MaxConnsMaxConnections, MinConnsMinConnections, MaxConnLifetimeJitterMaxConnectionLifetimeJitter; default init updated.
Database Factory / Wiring
internal/factories/database.go
Updated factory to read and prefer new fields (with fallbacks to deprecated names), and to pass renamed options into Postgres builder.
CLI Command & Flags
pkg/cmd/config.go, pkg/cmd/serve.go, pkg/cmd/flags/serve.go
Renamed CLI flags and viper bindings: database-max-connsdatabase-max-connections, database-min-connsdatabase-min-connections, database-max-conn-lifetime-jitterdatabase-max-connection-lifetime-jitter; updated env var names and deprecation/help text.
Postgres Options & Consts
pkg/database/postgres/options.go, pkg/database/postgres/consts.go
Renamed exported option functions and internal fields: MaxConnsMaxConnections, MinConnsMinConnections, MinIdleConnsMinIdleConnections, MaxConnLifetimeJitterMaxConnectionLifetimeJitter; MaxOpenConnections now forwards to MaxConnections.
Postgres Implementation & Tests
pkg/database/postgres/postgres.go, pkg/database/postgres/postgres_test.go
Internal field renames (e.g., maxConnsmaxConnections) and wiring updated for pool settings, jitter logic, and new pool timeouts; tests updated to use renamed fields and constructors.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

  • Heterogeneous but consistent renames across docs, config structs, CLI bindings, factory wiring, pkg options, and tests.
  • Areas needing extra attention:
    • internal/config/config.go — mapstructure tags and defaults vs. YAML/ENV mappings
    • internal/factories/database.go & pkg/database/postgres/* — precedence/fallback logic and pool option wiring
    • pkg/cmd/flags/serve.go & serve.go — Viper/flag/env binding correctness and deprecation messages
    • pkg/database/postgres/options.go — exported API renames and backward-compatible forwarding

Possibly related PRs

Poem

🐰 I nibble through keys, rename with glee,
From conns to connections, clearer to see,
Flags and docs hop in a tidy row,
v1.5.2 — onward we go! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately reflects both main objectives: updating the API version to v1.5.2 and refactoring database connection configuration field names from abbreviated to explicit forms (e.g., max_conns to max_connections).
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch update-api-version-and-config

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Nitpick comments (5)
internal/config/config.go (1)

169-183: Database config rename + deprecation strategy looks coherent

The Database struct now cleanly exposes MaxConnections, MinConnections, and MaxConnectionLifetimeJitter while keeping MaxOpenConnections and MaxIdleConnections as clearly-marked deprecated fields with documented mapping behavior. Mapstructure tags match the intended YAML keys and comments describe fallback semantics well. No issues from this file’s perspective.

If you plan a future breaking release, consider adding a TODO with a version target for removing the deprecated fields to make the deprecation lifecycle explicit.

pkg/cmd/flags/serve.go (1)

432-529: Viper bindings for renamed DB keys are correct; consider dual env aliases

The new bindings for database.max_connections, database.min_connections, and database.max_connection_lifetime_jitter correctly point at the renamed flags and keep existing env vars (PERMIFY_DATABASE_MAX_CONNS, PERMIFY_DATABASE_MIN_CONNS, PERMIFY_DATABASE_MAX_CONN_LIFETIME_JITTER) working for backward compatibility.

If you want to smooth the naming transition further, you could also bind the more literal env names (e.g. PERMIFY_DATABASE_MAX_CONNECTIONS, PERMIFY_DATABASE_MIN_CONNECTIONS, PERMIFY_DATABASE_MAX_CONNECTION_LIFETIME_JITTER) as additional aliases on the same keys, but this isn’t required.

pkg/database/postgres/postgres_test.go (1)

63-72: Tests correctly track renamed Postgres fields and options

The updates to use maxConnections, minConnections, minIdleConns, and maxConnectionLifetimeJitter match the updated Postgres struct and option helpers, and the backward-compat tests (fallback from maxIdleConnections when minConnections is 0, MaxOpenConnections mapping to maxConnections) exercise the intended behavior.

Only tiny nit: some test descriptions still refer to “MinConns/MaxConns” while the code now uses *Connections. You can update the strings later if you want them to mirror the field names exactly, but functionally these tests look solid.

Also applies to: 174-221, 233-277

pkg/database/postgres/postgres.go (1)

28-52: Connection pool option wiring and backward-compat behavior look consistent

The new internal fields and their usage (defaults, MinConnections/MaxIdleConnections fallback, MaxConnections mapping, and jitter behavior) are coherent and match the documented semantics. The jitter defaulting to 20% of max_connection_lifetime when unset is implemented as described, and the MinConnections vs MaxIdleConnections precedence is clear.

If you ever touch this again, you might consider standardizing the internal name maxConnectionLifeTime to maxConnectionLifetime for consistency with config/docs, but it’s not functionally problematic.

Also applies to: 66-77, 104-113, 124-128, 139-146

internal/factories/database.go (1)

22-27: Factory wiring for new pool settings and deprecated fields looks correct

The factory logic cleanly separates new knobs from deprecated ones and keeps backward compatibility:

  • MinConnections takes precedence, with MaxIdleConnections only acting as a fallback via the mapping in postgres.New.
  • MaxConnections is preferred, with MaxOpenConnections used as a compatibility path when MaxConnections is zero.
  • MinIdleConns is only honored when explicitly set, which matches the comments and the pgx mapping behavior.
  • Optional fields (HealthCheckPeriod, MaxConnectionLifetimeJitter, ConnectTimeout) are only applied when non-zero.

If you want to be ultra-explicit, you could guard the MaxOpenConnections path with conf.MaxOpenConnections > 0 as well, but the current behavior is logically sound and keeps “0 = unlimited” semantics intact.

Also applies to: 39-45, 47-50, 52-59, 61-64, 66-69, 71-80

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a18f763 and c4d8366.

⛔ Files ignored due to path filters (1)
  • pkg/pb/base/v1/openapi.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (16)
  • docs/api-reference/apidocs.swagger.json (1 hunks)
  • docs/api-reference/openapiv2/apidocs.swagger.json (1 hunks)
  • docs/performance-test/values.yaml (1 hunks)
  • docs/setting-up/configuration.mdx (3 hunks)
  • example.config.yaml (1 hunks)
  • internal/config/config.go (2 hunks)
  • internal/factories/database.go (3 hunks)
  • internal/info.go (1 hunks)
  • pkg/cmd/config.go (2 hunks)
  • pkg/cmd/flags/serve.go (3 hunks)
  • pkg/cmd/serve.go (1 hunks)
  • pkg/database/postgres/consts.go (1 hunks)
  • pkg/database/postgres/options.go (2 hunks)
  • pkg/database/postgres/postgres.go (5 hunks)
  • pkg/database/postgres/postgres_test.go (6 hunks)
  • proto/base/v1/openapi.proto (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
pkg/database/postgres/options.go (1)
pkg/database/postgres/postgres.go (1)
  • Postgres (22-52)
internal/factories/database.go (1)
pkg/database/postgres/options.go (5)
  • MinConnections (34-38)
  • MaxConnections (17-21)
  • MinIdleConnections (44-48)
  • HealthCheckPeriod (65-69)
  • MaxConnectionLifetimeJitter (72-76)
internal/config/config.go (1)
pkg/database/postgres/options.go (11)
  • MaxConnections (17-21)
  • MaxOpenConnections (12-14)
  • MaxIdleConnections (26-30)
  • MinConnections (34-38)
  • MaxConnectionIdleTime (51-55)
  • HealthCheckPeriod (65-69)
  • MaxConnectionLifetimeJitter (72-76)
  • ConnectTimeout (79-83)
  • MaxDataPerWrite (85-89)
  • MaxRetries (97-101)
  • WatchBufferSize (91-95)
pkg/cmd/config.go (2)
internal/config/config.go (1)
  • Database (160-184)
pkg/database/postgres/options.go (7)
  • MaxConnections (17-21)
  • MaxOpenConnections (12-14)
  • MaxIdleConnections (26-30)
  • MinConnections (34-38)
  • MaxConnectionIdleTime (51-55)
  • HealthCheckPeriod (65-69)
  • MaxConnectionLifetimeJitter (72-76)
pkg/database/postgres/postgres_test.go (2)
pkg/database/postgres/postgres.go (1)
  • Postgres (22-52)
pkg/database/postgres/options.go (5)
  • MaxConnections (17-21)
  • MinConnections (34-38)
  • MinIdleConnections (44-48)
  • MaxConnectionLifetimeJitter (72-76)
  • MaxOpenConnections (12-14)
pkg/cmd/serve.go (2)
internal/config/config.go (1)
  • Database (160-184)
pkg/database/postgres/options.go (7)
  • MaxConnections (17-21)
  • MaxOpenConnections (12-14)
  • MaxIdleConnections (26-30)
  • MinConnections (34-38)
  • MaxConnectionIdleTime (51-55)
  • HealthCheckPeriod (65-69)
  • MaxConnectionLifetimeJitter (72-76)
⏰ 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). (4)
  • GitHub Check: Scan Container for Vulnerabilities
  • GitHub Check: Test with Coverage
  • GitHub Check: Analyze (go)
  • GitHub Check: Test with Coverage
🔇 Additional comments (11)
pkg/database/postgres/consts.go (1)

5-5: Comment alignment with new naming looks good

The deprecation note and reference to MinConnections accurately reflect the new config naming while preserving backward-compatibility semantics. No code changes needed.

proto/base/v1/openapi.proto (1)

12-12: OpenAPI info.version bump is consistent

Updating info.version to v1.5.2 is consistent with the release and other artifacts. No further proto changes needed here.

internal/config/config.go (1)

357-372: Default database pool settings match the new naming and compatibility story

The updated defaults (MaxConnections = 0 with non-zero MaxOpenConnections / MaxIdleConnections) align with the comments about using old fields for backward compatibility while preferring the new names when explicitly set. This keeps existing configs working while nudging new users toward the new keys.

docs/api-reference/openapiv2/apidocs.swagger.json (1)

6-6: Swagger metadata version updated correctly

The info.version field now reports v1.5.2, in line with the rest of the version bump in this PR. No additional swagger changes appear necessary here.

internal/info.go (1)

25-27: Version constant bump is consistent with release metadata

Version = "v1.5.2" aligns with the OpenAPI and swagger metadata updates. The banner will now show the correct release version.

docs/api-reference/apidocs.swagger.json (1)

4-6: API version bump looks consistent

info.version updated to v1.5.2, matching the PR-wide version bump; no further changes needed here.

docs/performance-test/values.yaml (1)

103-119: DB pool key rename + deprecation docs look coherent

The rename to max_connections, min_connections, and max_connection_lifetime_jitter plus the deprecation notes for max_open_connections/max_idle_connections accurately describe the intended fallback behavior and align with the updated Postgres options/tests.

If you haven’t already, please double-check that docs/setting-up/configuration.mdx and any Helm chart templates reference the same key names, so users don’t see mixed *_conns / *_connections variants.

example.config.yaml (1)

76-90: Example DB config matches new naming and behavior

The example now correctly uses max_connections, min_connections, and max_connection_lifetime_jitter, and the deprecation comments for max_open_connections/max_idle_connections match the documented fallback behavior. The jitter note (20% of lifetime when 0) is clear.

Please ensure the implementation in pkg/database/postgres/postgres.go actually derives jitter as 20% of max_connection_lifetime when unset, so the comment stays truthful.

docs/setting-up/configuration.mdx (1)

97-101: Docs correctly reflect the new database configuration surface and compat paths

The updated YAML example, structure, glossary, and ENV mappings all line up with the new max_connections / min_connections naming, the added writer/reader URIs, and the deprecation story for max_open_connections and max_idle_connections. The explanation for max_connection_lifetime_jitter (defaulting to 20% of max_connection_lifetime when 0) matches the implementation.

Looks good and should make the new behavior much clearer to users.

Also applies to: 105-107, 397-421, 425-448, 452-475

pkg/database/postgres/options.go (1)

10-20: Option renames and deprecation shims are consistent with the Postgres config

The updated options cleanly expose the new naming while preserving old entry points:

  • MaxOpenConnections forwarding to MaxConnections keeps existing callers working.
  • MaxConnections, MinConnections, and MinIdleConnections map directly to the new internal fields, matching the factory and Postgres struct.
  • MaxIdleConnections is clearly marked as deprecated and only used as part of the MinConnections fallback path.
  • MaxConnectionLifetimeJitter plugs into the new jitter handling in postgres.New.

No functional issues from this refactor.

Also applies to: 23-30, 32-38, 40-48, 71-75

pkg/cmd/config.go (1)

93-96: LGTM! Good backward compatibility handling.

The CLI flags correctly reference the updated field names, and the deprecated flags include appropriate notices to guide users toward the new naming.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
docs/setting-up/configuration.mdx (1)

440-440: Use consistent config field naming in description.

The description mixes the pgxpool Go field name "MaxConnLifetime" with the config field name "max_connection_lifetime" within the same sentence. For clarity in user-facing documentation, consider using the config field name consistently.

Apply this diff to improve consistency:

-| [ ]      | max_connection_lifetime_jitter     | 0s      | Jitter added to MaxConnLifetime to prevent all connections from expiring at once. 0 means default to 20% of max_connection_lifetime. |
+| [ ]      | max_connection_lifetime_jitter     | 0s      | Jitter added to max_connection_lifetime to prevent all connections from expiring at once. 0 means default to 20% of max_connection_lifetime. |
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c4d8366 and 84d5416.

📒 Files selected for processing (4)
  • docs/setting-up/configuration.mdx (3 hunks)
  • pkg/cmd/config.go (2 hunks)
  • pkg/cmd/flags/serve.go (3 hunks)
  • pkg/cmd/serve.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • pkg/cmd/serve.go
  • pkg/cmd/flags/serve.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/cmd/config.go (2)
internal/config/config.go (1)
  • Database (160-184)
pkg/database/postgres/options.go (7)
  • MaxConnections (17-21)
  • MaxOpenConnections (12-14)
  • MaxIdleConnections (26-30)
  • MinConnections (34-38)
  • MaxConnectionIdleTime (51-55)
  • HealthCheckPeriod (65-69)
  • MaxConnectionLifetimeJitter (72-76)
⏰ 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). (4)
  • GitHub Check: Analyze (go)
  • GitHub Check: Scan Container for Vulnerabilities
  • GitHub Check: Test with Coverage
  • GitHub Check: Test with Coverage
🔇 Additional comments (6)
docs/setting-up/configuration.mdx (3)

97-105: LGTM! Clear deprecation notices.

The YAML example effectively demonstrates both the new and deprecated configuration fields with clear comments. This approach helps users understand the migration path while maintaining backward compatibility.


400-421: LGTM! Comprehensive structure documentation.

The structure section clearly shows all configuration options including the newly added writer.uri, reader.uri, and other database fields. The inclusion of both new and deprecated fields provides a complete reference.


452-476: LGTM! Consistent environment variable naming.

The environment variable naming consistently uses full forms (PERMIFY_DATABASE_MAX_CONNECTIONS, PERMIFY_DATABASE_MIN_CONNECTIONS, PERMIFY_DATABASE_MAX_CONNECTION_LIFETIME_JITTER) rather than abbreviated forms. The new variables for writer/reader URIs and other database settings are properly documented.

pkg/cmd/config.go (3)

93-96: LGTM! Clear flag definitions with deprecation notices.

The flag definitions properly introduce the new naming (database-max-connections, database-min-connections) while maintaining the deprecated flags (database-max-open-connections, database-max-idle-connections) with clear deprecation messages for backward compatibility.


101-101: LGTM! Help text correctly fixed.

The help text correctly describes that jitter is added to max_connection_lifetime, resolving the circular reference issue from the previous review.


223-231: LGTM! Environment variable naming is now consistent.

The environment variable names correctly use full forms (PERMIFY_DATABASE_MAX_CONNECTIONS, PERMIFY_DATABASE_MIN_CONNECTIONS, PERMIFY_DATABASE_MAX_CONNECTION_LIFETIME_JITTER) that match the flag naming convention and the documentation, resolving the inconsistency from the previous review.

These match the environment variable names documented in docs/setting-up/configuration.mdx lines 459, 462, and 467.

@tolgaozen tolgaozen merged commit 9d10112 into master Nov 15, 2025
13 checks passed
@tolgaozen tolgaozen deleted the update-api-version-and-config branch November 15, 2025 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants