LCORE-1062: new configuration options for quota runner#923
LCORE-1062: new configuration options for quota runner#923tisnik merged 3 commits intolightspeed-core:mainfrom
Conversation
WalkthroughAdds two new positive integer fields ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
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.
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)
src/models/config.py (1)
1370-1374: Fix mypy/pyright errors by passing all QuotaSchedulerConfiguration fields in default_factoryStatic type-checkers now consider
database_reconnection_countanddatabase_reconnection_delayrequired parameters onQuotaSchedulerConfiguration.__init__, soQuotaSchedulerConfiguration(period=1)triggers the reported errors.To keep behavior the same and satisfy mypy/pyright, pass all three fields explicitly in the default factory:
- scheduler: QuotaSchedulerConfiguration = Field( - default_factory=lambda: QuotaSchedulerConfiguration(period=1), + scheduler: QuotaSchedulerConfiguration = Field( + default_factory=lambda: QuotaSchedulerConfiguration( + period=1, + database_reconnection_count=10, + database_reconnection_delay=1, + ), title="Quota scheduler", description="Quota scheduler configuration", )This preserves the existing defaults used in tests and documentation while resolving the
Missing named argument/Arguments missing for parametersdiagnostics.docs/config.json (1)
893-906: Critical: Missing fields in QuotaSchedulerConfiguration schema.The
QuotaSchedulerConfigurationschema only defines theperiodfield, but is missing the two new fields that are the main focus of this PR:
database_reconnection_count(default: 10)database_reconnection_delay(default: 1)These fields are present in the source code (
src/models/config.pylines 1316-1339) and are being tested in the test file, but are absent from this OpenAPI schema documentation. This creates an inconsistency between the actual configuration model and its documented schema.Apply this diff to add the missing fields to the schema:
"QuotaSchedulerConfiguration": { "additionalProperties": false, "description": "Quota scheduler configuration.", "properties": { "period": { "default": 1, "description": "Quota scheduler period specified in seconds", "minimum": 0, "title": "Period", "type": "integer" + }, + "database_reconnection_count": { + "default": 10, + "description": "Database reconnection count on startup. When database for quota is not available on startup, the service tries to reconnect N times with specified delay.", + "minimum": 1, + "title": "Database reconnection count on startup", + "type": "integer" + }, + "database_reconnection_delay": { + "default": 1, + "description": "Database reconnection delay specified in seconds. When database for quota is not available on startup, the service tries to reconnect N times with specified delay.", + "minimum": 1, + "title": "Database reconnection delay", + "type": "integer" } }, "title": "QuotaSchedulerConfiguration", "type": "object" },
🧹 Nitpick comments (1)
docs/openapi.json (1)
6600-6620: New reconnection fields look correct; please confirm zero-handling matches backend validationThe added
database_reconnection_countanddatabase_reconnection_delayfields are well-positioned next toperiod, use consistent integer +exclusiveMinimum: 0.0constraints, and their titles/descriptions clearly explain the startup retry behavior. This aligns with the PR intent for quota DB reconnection control.One thing to double-check: using
exclusiveMinimum: 0.0means OpenAPI declares only values> 0as valid. If the Pydantic model forQuotaSchedulerConfigurationallows0(e.g., to mean “no retries” or “no delay”) withge=0/min_value=0, then the generated OpenAPI will be stricter than the actual config rules. If you intend to allow0, switch these to a non‑exclusiveminimum: 0.0; if you intend to forbid0, ensure the Python-side validators/tests also reject zero for both fields so behavior and docs stay aligned.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
docs/config.svgis excluded by!**/*.svg
📒 Files selected for processing (9)
docs/config.html(2 hunks)docs/config.json(6 hunks)docs/config.md(1 hunks)docs/config.puml(1 hunks)docs/openapi.json(1 hunks)docs/openapi.md(1 hunks)src/models/config.py(1 hunks)tests/unit/models/config/test_dump_configuration.py(4 hunks)tests/unit/models/config/test_quota_scheduler_config.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/models/config.py
src/models/config.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/config.py: All configuration must use Pydantic models extendingConfigurationBasewithextra="forbid"to reject unknown fields
Use type hintsOptional[FilePath],PositiveInt,SecretStrfor Pydantic configuration models
Files:
src/models/config.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Use@field_validatorand@model_validatorfor custom validation in Pydantic models
Pydantic configuration classes must extendConfigurationBase; data models must extendBaseModel
Files:
src/models/config.py
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests; do not use unittest framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_quota_scheduler_config.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/models/config/test_dump_configuration.pytests/unit/models/config/test_quota_scheduler_config.py
🧬 Code graph analysis (1)
tests/unit/models/config/test_quota_scheduler_config.py (1)
src/models/config.py (1)
QuotaSchedulerConfiguration(1317-1340)
🪛 GitHub Actions: Pyright
src/models/config.py
[error] 1371-1371: pyright: Arguments missing for parameters "database_reconnection_count", "database_reconnection_delay" (reportCallIssue)
🪛 GitHub Actions: Type checks
src/models/config.py
[error] 1371-1371: Mypy: Missing named argument 'database_reconnection_count' for 'QuotaSchedulerConfiguration'. Command: uv run mypy --explicit-package-bases --disallow-untyped-calls --disallow-untyped-defs --disallow-incomplete-defs --ignore-missing-imports --disable-error-code attr-defined src/
[error] 1371-1371: Mypy: Missing named argument 'database_reconnection_delay' for 'QuotaSchedulerConfiguration'. Command: uv run mypy --explicit-package-bases --disallow-untyped-calls --disallow-untyped-defs --disallow-incomplete-defs --ignore-missing-imports --disable-error-code attr-defined src/
⏰ 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). (6)
- GitHub Check: build-pr
- GitHub Check: E2E: library mode / azure
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: server mode / azure
- GitHub Check: E2E: server mode / ci
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
🔇 Additional comments (9)
docs/config.puml (1)
159-162: Quota scheduler fields documented consistently
database_reconnection_countanddatabase_reconnection_delayhere match the new fields onQuotaSchedulerConfigurationinsrc/models/config.pyand their placement beforeperiodis consistent with other docs. No further changes needed.src/models/config.py (1)
1317-1340: New quota reconnection fields look correct and well-typedUsing
PositiveIntwith defaults (10 count, 1 second delay) and explicit descriptions fordatabase_reconnection_countanddatabase_reconnection_delayaligns with the rest of the configuration models and the intended startup reconnection behavior. No issues from a modeling/validation perspective.docs/openapi.md (1)
4352-4357: OpenAPI schema updated correctly for new quota reconnection fieldsThe
QuotaSchedulerConfigurationcomponent now includesdatabase_reconnection_countanddatabase_reconnection_delaywith descriptions matching the model and other docs. This keeps the OpenAPI surface in sync with the Python configuration.tests/unit/models/config/test_dump_configuration.py (1)
186-190: Dump configuration tests correctly assert new scheduler fieldsThe updated expectations for
quota_handlers.scheduler(includingdatabase_reconnection_countanddatabase_reconnection_delayalongsideperiod) match the newQuotaSchedulerConfigurationmodel and ensure the dump format is locked in for both default and custom scheduler periods. Looks good.Also applies to: 505-509, 689-693, 859-863
docs/config.md (1)
420-425: Config schema docs aligned with new QuotaSchedulerConfiguration fieldsThe
QuotaSchedulerConfigurationtable now documentsdatabase_reconnection_countanddatabase_reconnection_delaywith accurate descriptions. This keeps the human-readable config schema in sync with the code and OpenAPI docs.docs/config.html (1)
1119-1123: HTML config docs correctly include new quota reconnection fieldsThe QuotaSchedulerConfiguration table now has a proper
<colgroup>and rows fordatabase_reconnection_countanddatabase_reconnection_delaywith descriptions consistent with other docs and the model. No issues.Also applies to: 1137-1150
docs/config.json (1)
9-29: Verify that APIKeyTokenConfiguration changes belong in this PR.The PR objectives state this PR "Adds new configuration options for the quota runner (LCORE-1062)", but this new
APIKeyTokenConfigurationschema appears unrelated to quota configuration. Please confirm whether these authentication-related changes should be part of this PR or split into a separate PR for better traceability.tests/unit/models/config/test_quota_scheduler_config.py (2)
10-17: LGTM!The test correctly verifies the default values for the new configuration fields, including the quota runner reconnection parameters.
20-30: LGTM!The test correctly validates custom configuration values for all fields, including the new reconnection parameters.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/unit/models/config/test_quota_scheduler_config.py (2)
45-48: Test looks correct, but consider adding coverage for zero value.The negative value validation test is correct. However, since
database_reconnection_countis aPositiveInt(must be > 0), consider adding a test case for zero values as well, similar to the existingtest_quota_scheduler_custom_configuration_zero_periodtest.Example:
def test_quota_scheduler_custom_configuration_zero_reconnection_count() -> None: """Test that zero database_reconnection_count value raises ValidationError.""" with pytest.raises(ValidationError, match="Input should be greater than 0"): QuotaSchedulerConfiguration(database_reconnection_count=0)
51-54: Test looks correct, but consider adding coverage for zero value.The negative value validation test is correct. However, since
database_reconnection_delayis aPositiveInt(must be > 0), consider adding a test case for zero values as well, similar to the existingtest_quota_scheduler_custom_configuration_zero_periodtest.Example:
def test_quota_scheduler_custom_configuration_zero_reconnection_delay() -> None: """Test that zero database_reconnection_delay value raises ValidationError.""" with pytest.raises(ValidationError, match="Input should be greater than 0"): QuotaSchedulerConfiguration(database_reconnection_delay=0)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/models/config.py(2 hunks)tests/unit/models/config/test_quota_scheduler_config.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/models/config.py
🧰 Additional context used
📓 Path-based instructions (2)
tests/{unit,integration}/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/{unit,integration}/**/*.py: Use pytest for all unit and integration tests; do not use unittest framework
Unit tests must achieve 60% code coverage; integration tests must achieve 10% coverage
Files:
tests/unit/models/config/test_quota_scheduler_config.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
Use
pytest-mockwith AsyncMock objects for mocking in tests
Files:
tests/unit/models/config/test_quota_scheduler_config.py
🧬 Code graph analysis (1)
tests/unit/models/config/test_quota_scheduler_config.py (1)
src/models/config.py (1)
QuotaSchedulerConfiguration(1317-1340)
⏰ 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). (6)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: build-pr
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: server mode / azure
- GitHub Check: E2E: server mode / ci
- GitHub Check: E2E: library mode / azure
🔇 Additional comments (2)
tests/unit/models/config/test_quota_scheduler_config.py (2)
16-17: LGTM!The default value assertions correctly match the model defaults defined in
src/models/config.py(database_reconnection_count=10, database_reconnection_delay=1).
22-30: LGTM!The custom configuration test properly validates all three fields with appropriate test values.
ca2f2af to
defcde6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/models/config.py (1)
1334-1340: LGTM! Reconnection delay properly configured.The field correctly uses
PositiveIntwith a sensible default of 1 second. The implementation aligns with coding guidelines and complements the reconnection count field.Optional: Consider making the description slightly more distinct from
database_reconnection_countto improve readability. For example:- database_reconnection_delay: PositiveInt = Field( - 1, - title="Database reconnection delay", - description="Database reconnection delay specified in seconds. When database for " - "quota is not available on startup, the service tries to reconnect N " - "times with specified delay.", - ) + database_reconnection_delay: PositiveInt = Field( + 1, + title="Database reconnection delay", + description="Delay in seconds between reconnection attempts when the quota " + "database is unavailable on startup.", + )
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/models/config.py(2 hunks)tests/unit/models/config/test_quota_scheduler_config.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/models/config/test_quota_scheduler_config.py
🧰 Additional context used
📓 Path-based instructions (3)
src/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/**/*.py: Use absolute imports for internal modules in LCS project (e.g.,from auth import get_auth_dependency)
All modules must start with descriptive docstrings explaining their purpose
Uselogger = logging.getLogger(__name__)pattern for module logging
All functions must include complete type annotations for parameters and return types, using modern syntax (str | int) andOptional[Type]orType | None
All functions must have docstrings with brief descriptions following Google Python docstring conventions
Function names must use snake_case with descriptive, action-oriented names (get_, validate_, check_)
Avoid in-place parameter modification anti-patterns; return new data structures instead of modifying input parameters
Useasync deffor I/O operations and external API calls
All classes must include descriptive docstrings explaining their purpose following Google Python docstring conventions
Class names must use PascalCase with descriptive names and standard suffixes:Configurationfor config classes,Error/Exceptionfor exceptions,Resolverfor strategy patterns,Interfacefor abstract base classes
Abstract classes must use ABC with@abstractmethoddecorators
Include complete type annotations for all class attributes in Python classes
Useimport loggingand module logger pattern with standard log levels: debug, info, warning, error
Files:
src/models/config.py
src/models/config.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/config.py: All configuration must use Pydantic models extendingConfigurationBasewithextra="forbid"to reject unknown fields
Use type hintsOptional[FilePath],PositiveInt,SecretStrfor Pydantic configuration models
Files:
src/models/config.py
src/models/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
src/models/**/*.py: Use@field_validatorand@model_validatorfor custom validation in Pydantic models
Pydantic configuration classes must extendConfigurationBase; data models must extendBaseModel
Files:
src/models/config.py
⏰ 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). (6)
- GitHub Check: Konflux kflux-prd-rh02 / lightspeed-stack-on-pull-request
- GitHub Check: E2E: server mode / ci
- GitHub Check: build-pr
- GitHub Check: E2E: library mode / azure
- GitHub Check: E2E: library mode / ci
- GitHub Check: E2E: server mode / azure
🔇 Additional comments (2)
src/models/config.py (2)
1326-1332: LGTM! Well-structured reconnection count configuration.The field is correctly typed with
PositiveInt, ensuring only positive values are accepted. The default value of 10 reconnection attempts is reasonable, and the description clearly explains its purpose.
1371-1373: LGTM! Default factory correctly updated.The default factory properly initializes all three
QuotaSchedulerConfigurationparameters with values that match the Field defaults. This ensures consistency across the configuration model.
Description
LCORE-1062: new configuration options for quota runner
Type of change
Tools used to create PR
Identify any AI code assistants used in this PR (for transparency and review context)
Related Tickets & Documents
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.