feat(ledger): add configurable default and max page sizes #214
Conversation
|
Warning Rate limit exceeded@gfyrag has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 46 minutes and 23 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (2)
WalkthroughThe pull request introduces documentation updates for the Settings Custom Resource Definition (CRD) in Operator v2. New configuration settings are added for job security contexts, including run-as configurations for containers and init-containers. Additionally, ledger API pagination settings are introduced to control default and maximum page sizes. The documentation structure is refined to provide more explicit type descriptions, enhancing clarity for users configuring the system. Changes
Sequence DiagramsequenceDiagram
participant Settings as Settings CRD
participant Deployment as Ledger Deployment
participant Container as Ledger Container
Settings->>Deployment: Configure page size settings
Deployment->>Container: Set environment variables
Container->>Container: Apply pagination configuration
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
internal/resources/ledgers/deployments.go (1)
174-182: Add validation for default page size.Consider adding validation to ensure the default page size is a positive integer. This will prevent potential issues with invalid values.
defaultPageSize, err := settings.GetInt(ctx, stack.Name, "ledger", "api", "default-page-size") if err != nil { return fmt.Errorf("failed to get default page size: %w", err) } -if defaultPageSize != nil { +if defaultPageSize != nil && *defaultPageSize > 0 { container.Env = append(container.Env, core.Env("DEFAULT_PAGE_SIZE", fmt.Sprint(*defaultPageSize)), ) +} else if defaultPageSize != nil { + return fmt.Errorf("default page size must be positive, got: %d", *defaultPageSize) }docs/09-Configuration reference/01-Settings.md (2)
26-27: Enhance documentation for pagination settings.Consider adding:
- Example values for both settings
- More detailed descriptions explaining the impact of these settings
- Recommended value ranges or best practices
-| ledger.api.default-page-size | Int | | Default api page size | -| ledger.api.max-page-size | Int | | Max page size | +| ledger.api.default-page-size | Int | 50 | Default number of items per page in API responses. Must be a positive integer. | +| ledger.api.max-page-size | Int | 1000 | Maximum allowed items per page in API responses. Must be greater than or equal to default page size. |
36-37: Fix markdown formatting issues.
- Add blank lines around the table
- Replace the plus sign with an asterisk for consistent list formatting
| caddy.image | string | | Caddy image | + - + | jobs.`<owner-kind>`.init-containers.`<container-name>`.run-as | Map | user=X, group=X | Configure the security context for init containers in jobs by specifying the user and group IDs to run as | +| jobs.`<owner-kind>`.init-containers.`<container-name>`.run-as | Map | user=X, group=X | Configure the security context for init containers in jobs by specifying the user and group IDs to run as | + | jobs.`<owner-kind>`.containers.`<container-name>`.run-as | Map | user=X, group=X | Configure the security context for containers in jobs by specifying the user and group IDs to run as |🧰 Tools
🪛 Markdownlint (0.37.0)
37-37: Expected: asterisk; Actual: plus
Unordered list style(MD004, ul-style)
37-37: Expected: 0; Actual: 1
Unordered list indentation(MD007, ul-indent)
36-36: null
Tables should be surrounded by blank lines(MD058, blanks-around-tables)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/09-Configuration reference/01-Settings.md(1 hunks)internal/resources/ledgers/deployments.go(1 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
docs/09-Configuration reference/01-Settings.md
37-37: Expected: asterisk; Actual: plus
Unordered list style
(MD004, ul-style)
37-37: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
36-36: null
Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
| maxPageSize, err := settings.GetInt(ctx, stack.Name, "ledger", "api", "max-page-size") | ||
| if err != nil { | ||
| return fmt.Errorf("failed to get max page size: %w", err) | ||
| } | ||
| if maxPageSize != nil { | ||
| container.Env = append(container.Env, | ||
| core.Env("MAX_PAGE_SIZE", fmt.Sprint(*maxPageSize)), | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for max page size and its relationship with default page size.
Consider adding validation to ensure:
- The max page size is a positive integer
- The max page size is greater than or equal to the default page size (when both are set)
maxPageSize, err := settings.GetInt(ctx, stack.Name, "ledger", "api", "max-page-size")
if err != nil {
return fmt.Errorf("failed to get max page size: %w", err)
}
-if maxPageSize != nil {
+if maxPageSize != nil && *maxPageSize > 0 {
+ if defaultPageSize != nil && *maxPageSize < *defaultPageSize {
+ return fmt.Errorf("max page size (%d) must be greater than or equal to default page size (%d)", *maxPageSize, *defaultPageSize)
+ }
container.Env = append(container.Env,
core.Env("MAX_PAGE_SIZE", fmt.Sprint(*maxPageSize)),
)
+} else if maxPageSize != nil {
+ return fmt.Errorf("max page size must be positive, got: %d", *maxPageSize)
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
api/formance.com/v1beta1/broker_types.go (1)
43-44: Consider aligning kubebuilder validation with constant referencesWhile the documentation update to use constant references (
ModeOneStreamByServiceandModeOneStreamByStack) improves maintainability, the kubebuilder validation still uses string literals. Consider updating the validation to use the constants for consistency.-//+kubebuilder:validation:Enum:={OneStreamByService, OneStreamByStack} +//+kubebuilder:validation:Enum:={ModeOneStreamByService, ModeOneStreamByStack}
gfyrag
force-pushed
the
feat/ledger-page-sizes
branch
from
fa111fb to
160d0e3
Compare
Fixes ENG-1577