-
-
Notifications
You must be signed in to change notification settings - Fork 278
docs: update API version to v1.5.2 and refactor connection settings i… #2610
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis 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: Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this 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 coherentThe
Databasestruct now cleanly exposesMaxConnections,MinConnections, andMaxConnectionLifetimeJitterwhile keepingMaxOpenConnectionsandMaxIdleConnectionsas 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 aliasesThe new bindings for
database.max_connections,database.min_connections, anddatabase.max_connection_lifetime_jittercorrectly 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 optionsThe updates to use
maxConnections,minConnections,minIdleConns, andmaxConnectionLifetimeJittermatch the updatedPostgresstruct and option helpers, and the backward-compat tests (fallback frommaxIdleConnectionswhenminConnectionsis 0,MaxOpenConnectionsmapping tomaxConnections) 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 consistentThe 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_lifetimewhen 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
maxConnectionLifeTimetomaxConnectionLifetimefor 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 correctThe factory logic cleanly separates new knobs from deprecated ones and keeps backward compatibility:
MinConnectionstakes precedence, withMaxIdleConnectionsonly acting as a fallback via the mapping inpostgres.New.MaxConnectionsis preferred, withMaxOpenConnectionsused as a compatibility path whenMaxConnectionsis zero.MinIdleConnsis 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
MaxOpenConnectionspath withconf.MaxOpenConnections > 0as 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
⛔ Files ignored due to path filters (1)
pkg/pb/base/v1/openapi.pb.gois 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 goodThe deprecation note and reference to
MinConnectionsaccurately 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 consistentUpdating
info.versiontov1.5.2is 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 storyThe updated defaults (
MaxConnections = 0with non-zeroMaxOpenConnections/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 correctlyThe
info.versionfield now reportsv1.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.versionupdated tov1.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 coherentThe rename to
max_connections,min_connections, andmax_connection_lifetime_jitterplus the deprecation notes formax_open_connections/max_idle_connectionsaccurately 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.mdxand any Helm chart templates reference the same key names, so users don’t see mixed*_conns/*_connectionsvariants.example.config.yaml (1)
76-90: Example DB config matches new naming and behaviorThe example now correctly uses
max_connections,min_connections, andmax_connection_lifetime_jitter, and the deprecation comments formax_open_connections/max_idle_connectionsmatch the documented fallback behavior. The jitter note (20% of lifetime when 0) is clear.Please ensure the implementation in
pkg/database/postgres/postgres.goactually derives jitter as 20% ofmax_connection_lifetimewhen unset, so the comment stays truthful.docs/setting-up/configuration.mdx (1)
97-101: Docs correctly reflect the new database configuration surface and compat pathsThe updated YAML example, structure, glossary, and ENV mappings all line up with the new
max_connections/min_connectionsnaming, the added writer/reader URIs, and the deprecation story formax_open_connectionsandmax_idle_connections. The explanation formax_connection_lifetime_jitter(defaulting to 20% ofmax_connection_lifetimewhen 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 configThe updated options cleanly expose the new naming while preserving old entry points:
MaxOpenConnectionsforwarding toMaxConnectionskeeps existing callers working.MaxConnections,MinConnections, andMinIdleConnectionsmap directly to the new internal fields, matching the factory andPostgresstruct.MaxIdleConnectionsis clearly marked as deprecated and only used as part of the MinConnections fallback path.MaxConnectionLifetimeJitterplugs into the new jitter handling inpostgres.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.
There was a problem hiding this 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
📒 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.mdxlines 459, 462, and 467.
…n YAML files
Summary by CodeRabbit
New Features
Improvements