Multiple data store support - #202
Merged
Merged
Conversation
# Conflicts: # hub_adapter/integration_test.py # hub_adapter/routers/kong.py # tests/test_autostart.py
Closed
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThis PR refactors Kong datastore and project integration around tags, adds S3 configuration and health probing, standardizes Kong errors, and introduces configurable background cleanup for stale analysis consumers. ChangesKong identity and error contracts
Datastore services and links
Project, analysis, and probing flow
Consumer cleanup lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ServerLifespan
participant KongCleanupManager
participant KongConsumerReaper
participant Hub
participant Kong
ServerLifespan->>KongCleanupManager: start cleanup loop
KongCleanupManager->>KongConsumerReaper: run sweep
KongConsumerReaper->>Kong: list analysis consumers
KongConsumerReaper->>Hub: fetch analysis execution status
KongConsumerReaper->>Kong: delete terminal analysis consumers
ServerLifespan->>KongCleanupManager: stop cleanup loop
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
brucetony
changed the title
brucetony
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (8)
hub_adapter/maintenance.py (2)
65-77: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSync Kong client call blocks the event loop.
get_analysesis a synchronous function performing HTTP I/O viakong_admin_client; calling it directly inside this coroutine stalls the whole event loop for the duration of the Kong request (including timeouts). Considerawait run_in_threadpool(get_analyses, self.settings).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hub_adapter/maintenance.py` around lines 65 - 77, Update the coroutine’s get_analyses call to execute via await run_in_threadpool(get_analyses, self.settings), preserving the existing ApiException/HTTPException handling and deleted return path.
33-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConstant name doesn't match its use.
EXECUTED_AFTER_RUNNING_GRACEis applied only tofailed/stoppedanalyses that were previously seenexecuting(Line 131);executeddeletes immediately with no grace. Something likeTERMINAL_AFTER_EXECUTING_GRACEwould read truer.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hub_adapter/maintenance.py` around lines 33 - 37, Rename EXECUTED_AFTER_RUNNING_GRACE to a name that reflects its use for failed or stopped analyses previously observed as executing, such as TERMINAL_AFTER_EXECUTING_GRACE, and update all references including the logic near the terminal-status handling. Keep the immediate deletion behavior for executed analyses unchanged.hub_adapter/server.py (1)
52-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent tag value formatting. Only the PodOrc entry uses
.value; every other entry intags_metadatapasses theServiceTagmember. SinceServiceTagis astrEnum both serialize the same, so either revert this or apply.valueto all entries for consistency.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hub_adapter/server.py` at line 52, Standardize the tag value representation in the tags_metadata definition: update the PodOrc entry using ServiceTag.PODORC.value to match the other ServiceTag entries, or consistently convert every entry to .value. Preserve the existing tag values and choose one representation throughout tags_metadata.hub_adapter/routers/kong.py (2)
537-537: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMutable default for
protocols.Ruff B006. Use
Noneand fall back toDEFAULT_PROTOCOLS(already defined on Line 88) inside the body, matchinglink_project_to_datastore.♻️ Proposed change
- ] = ["http"], + ] = None,and pass
protocols=protocols or DEFAULT_PROTOCOLStolink_project_to_datastore.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hub_adapter/routers/kong.py` at line 537, Update the function parameter defining protocols to use None instead of the mutable list default, then inside the function pass protocols or DEFAULT_PROTOCOLS to link_project_to_datastore, matching the existing link_project_to_datastore pattern.Source: Linters/SAST tools
763-763: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winStringify the ACL group for consistency.
ensure_health_consumer(Line 837) and the route ACL (Line 502) both usestr(project_id). Here the raw value is passed, so auuid.UUIDcaller would serialize to a group that doesn't match the route's allow-list.♻️ Proposed change
- group=project_id, + group=str(project_id),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hub_adapter/routers/kong.py` at line 763, Update the ACL group argument in the relevant ensure_health_consumer call to pass str(project_id), matching the route ACL and other ensure_health_consumer usage so UUID project IDs serialize consistently.hub_adapter/utils.py (1)
152-153: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComment contradicts the rule it documents.
The regex deliberately excludes
,and/(they'd corrupt tag filters and link paths). The current wording reads as if they were allowed.📝 Suggested wording
-# Kong can use ',' and '/' in tags +# Kong treats ',' as a tag-filter separator and '/' is meaningful in route paths, so both are excluded _NAME_PATTERN = re.compile(r"^[a-zA-Z0-9._~-]+$")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hub_adapter/utils.py` around lines 152 - 153, Update the comment above _NAME_PATTERN to state that Kong cannot safely use commas or slashes in tags because they would corrupt tag filters and link paths; keep the regex unchanged.tests/router_tests/test_meta.py (1)
187-187: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove leftover commented-out assertion.
Dead code artifact from editing; can be dropped.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/router_tests/test_meta.py` at line 187, Remove the leftover commented-out assertion from the test at the indicated location in tests/router_tests/test_meta.py, leaving the surrounding test logic unchanged.hub_adapter/errors.py (1)
155-302: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
SERVICEfor the detail service key.The imported
SERVICEconstant already resolves to"service", so these repeated"service": "Kong"entries can useSERVICE: "Kong"instead of hardcoding the literal key.♻️ Example fix pattern
- "service": "Kong", + SERVICE: "Kong",Apply consistently to all new classes in this range.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hub_adapter/errors.py` around lines 155 - 302, Update every detail dictionary in the new error classes from KongDataStoreLinkedError through KongUpstreamError to use the imported SERVICE constant as the service key instead of the hardcoded "service" literal, preserving the existing "Kong" value and all other fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hub_adapter/conf.py`:
- Around line 18-25: Constrain the interval field in KongCleanupSettings to
accept only positive values, rejecting zero and negative values during model
validation. Preserve the optional configuration and existing default while
ensuring all KongCleanupManager.start() and router restart paths receive a valid
sleep interval.
In `@hub_adapter/errors.py`:
- Around line 290-300: The KongUpstreamError constructor must stop exposing raw
upstream response text and must not always attach WWW-Authenticate. Sanitize and
truncate the message before storing it in detail, and only include the
authentication header for status codes or responses that genuinely require a
bearer challenge; otherwise omit it while preserving the existing error details.
In `@hub_adapter/integration_test.py`:
- Line 205: Update the exception tuple in the cleanup handler to reference the
imported HTTP client alias httpx2 instead of the undefined httpx name,
preserving handling for HTTPException and HTTP status errors without masking the
original cleanup failure.
In `@hub_adapter/maintenance.py`:
- Around line 190-201: Move KongConsumerReaper construction from before the loop
into the while loop’s try block so initialization failures are handled by the
existing cleanup error path. Ensure KongConsumerReaper() exceptions produce the
kong_cleanup.error log and do not terminate _run_cleanup without retrying
subsequent cleanup cycles.
- Around line 141-152: Update _delete to catch both HTTPException and
ApiException from delete_analysis, matching the exception handling used by
sweep(). Keep the existing kong_cleanup.delete_error logging and False return
behavior so Kong-level failures do not escape and abort subsequent cleanup work.
- Around line 173-188: Enforce a positive minimum for kong_cleanup.interval in
the UserSettings/Pydantic settings model, and ensure KongCleanupManager.start
uses a safe validated value when loading persisted settings during reloads or
restarts. Prevent interval 0 or other invalid persisted values from reaching
asyncio.sleep or creating a tight cleanup loop.
In `@hub_adapter/routers/kong.py`:
- Around line 460-481: Update the route creation flow around CreateRouteRequest
to leave name unset instead of using route_name, avoiding global name collisions
and optional-service-name failures. Import and use link_path(project_id, svc.id)
for the sole paths entry so each project/datastore link receives a
project-specific route path, while preserving the existing tags and other route
settings.
- Around line 507-517: Extend the route-creation flow around
create_plugin_for_route so failures from either key-auth or ACL plugin creation
delete the newly created route before propagating the exception. Reuse the
existing Kong route deletion logic and preserve the probe_connection rollback
behavior, ensuring no route remains when plugin setup is incomplete.
In `@tests/test_maintenance.py`:
- Around line 152-155: Update every patch target in
test_delete_success_clears_history and the other tests in this file to remove
the stray “.py” segment, changing references from
hub_adapter.maintenance.py.<symbol> to hub_adapter.maintenance.<symbol>. Apply
this consistently to all patch decorators without changing the patched symbols
or test behavior.
---
Nitpick comments:
In `@hub_adapter/errors.py`:
- Around line 155-302: Update every detail dictionary in the new error classes
from KongDataStoreLinkedError through KongUpstreamError to use the imported
SERVICE constant as the service key instead of the hardcoded "service" literal,
preserving the existing "Kong" value and all other fields.
In `@hub_adapter/maintenance.py`:
- Around line 65-77: Update the coroutine’s get_analyses call to execute via
await run_in_threadpool(get_analyses, self.settings), preserving the existing
ApiException/HTTPException handling and deleted return path.
- Around line 33-37: Rename EXECUTED_AFTER_RUNNING_GRACE to a name that reflects
its use for failed or stopped analyses previously observed as executing, such as
TERMINAL_AFTER_EXECUTING_GRACE, and update all references including the logic
near the terminal-status handling. Keep the immediate deletion behavior for
executed analyses unchanged.
In `@hub_adapter/routers/kong.py`:
- Line 537: Update the function parameter defining protocols to use None instead
of the mutable list default, then inside the function pass protocols or
DEFAULT_PROTOCOLS to link_project_to_datastore, matching the existing
link_project_to_datastore pattern.
- Line 763: Update the ACL group argument in the relevant ensure_health_consumer
call to pass str(project_id), matching the route ACL and other
ensure_health_consumer usage so UUID project IDs serialize consistently.
In `@hub_adapter/server.py`:
- Line 52: Standardize the tag value representation in the tags_metadata
definition: update the PodOrc entry using ServiceTag.PODORC.value to match the
other ServiceTag entries, or consistently convert every entry to .value.
Preserve the existing tag values and choose one representation throughout
tags_metadata.
In `@hub_adapter/utils.py`:
- Around line 152-153: Update the comment above _NAME_PATTERN to state that Kong
cannot safely use commas or slashes in tags because they would corrupt tag
filters and link paths; keep the regex unchanged.
In `@tests/router_tests/test_meta.py`:
- Line 187: Remove the leftover commented-out assertion from the test at the
indicated location in tests/router_tests/test_meta.py, leaving the surrounding
test logic unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 33c44365-ff2e-4fe8-8339-79930e3a0a0e
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
hub_adapter/autostart.pyhub_adapter/conf.pyhub_adapter/constants.pyhub_adapter/errors.pyhub_adapter/integration_test.pyhub_adapter/maintenance.pyhub_adapter/routers/kong.pyhub_adapter/routers/meta.pyhub_adapter/routers/node.pyhub_adapter/routers/storage.pyhub_adapter/schemas/kong.pyhub_adapter/schemas/logs.pyhub_adapter/server.pyhub_adapter/utils.pypyproject.tomltests/constants.pytests/router_tests/routes.pytests/router_tests/test_kong.pytests/router_tests/test_meta.pytests/test_autostart.pytests/test_errors.pytests/test_maintenance.pytests/test_utils.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit