Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughResolves RAG image and validates compatibility, creates a shared Changes
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Duplication | 10 |
TIP This summary will be updated as you push new changes. Give us feedback
Contributor
Author
|
@coderabbitai review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/internal/orchestrator/swarm/service_instance_spec.go`:
- Around line 69-79: The RAG branch only waits for RAGConfigResource but must
also wait for the keys resource; update the dependency construction so when
s.ServiceSpec.ServiceType == "rag" you also append the keys resource identifier
(e.g. call RAGServiceKeysResourceIdentifier(s.ServiceInstanceID)) to deps
alongside RAGConfigResourceIdentifier(s.ServiceInstanceID), keeping the existing
ServiceUserRoleIdentifier logic intact.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9cfadc5c-ecef-4950-9d25-2af885ee4fd2
📒 Files selected for processing (7)
server/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/rag_service_keys_resource_test.goserver/internal/orchestrator/swarm/rag_service_user_role_test.goserver/internal/orchestrator/swarm/service_instance.goserver/internal/orchestrator/swarm/service_instance_spec.goserver/internal/orchestrator/swarm/service_spec.goserver/internal/workflows/plan_update.go
Contributor
Author
|
@coderabbitai review |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
tsivaprasad
force-pushed
the
PLAT-492-rag-service-container-deployment
branch
from
97d40a1 to
060fbb3
Compare
tsivaprasad
force-pushed
the
PLAT-491-rag-service-yaml-config-generation-swarm-config
branch
2 times, most recently
from
2719035 to
a86d831
Compare
tsivaprasad
changed the base branch from
PLAT-491-rag-service-yaml-config-generation-swarm-config
to
main
tsivaprasad
force-pushed
the
PLAT-492-rag-service-container-deployment
branch
from
060fbb3 to
b0e3c9a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/internal/orchestrator/swarm/rag_service_user_role_test.go (1)
85-120: Assert the new RAG resource payloads, not just their positions.These checks only validate type/order. They would still pass if
ServiceInstanceSpecResourcedropped fields likeDatabaseNetworkIDorDataDirID, or ifServiceInstanceResource.ServiceTypestopped being"rag". Please deserialize those tail resources and assert the RAG-specific fields introduced in this PR.Also applies to: 149-207, 236-271
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/swarm/rag_service_user_role_test.go` around lines 85 - 120, The test currently only asserts resource ordering/types (result.Resources) but must also deserialize and assert RAG-specific payload fields: for the ServiceInstanceSpecResource (use symbol ServiceInstanceSpecResource or the type used in the diff) deserialize its payload and assert fields like DatabaseNetworkID, DataDirID, and any RAG-specific spec values are present and correct; for the ServiceInstanceResource (symbol ServiceInstanceResource) deserialize and assert ServiceType == "rag" and other service-level fields introduced by this PR; likewise deserialize any RAG config/keys resources (ResourceTypeRAGConfig, ResourceTypeRAGServiceKeys) and assert expected config values and key presence; update the checks around result.Resources[1], [5], and [6] (and the other ranges noted) to include these payload assertions rather than relying only on index/type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/internal/orchestrator/swarm/orchestrator.go`:
- Around line 656-660: After obtaining serviceImage from
o.serviceVersions.GetServiceImage, run the same compatibility gate used in
generateMCPInstanceResources by calling ValidateCompatibility with the resolved
serviceImage and the target database/postgres version from the spec (the same
field used in generateMCPInstanceResources); if ValidateCompatibility reports
incompatible, return the error before planning resources so unsupported
RAG/Postgres combinations are rejected early. Ensure you reference the existing
ValidateCompatibility(...) call pattern from generateMCPInstanceResources and
perform this check immediately after obtaining serviceImage (the variable
serviceImage) and before any resource planning logic.
---
Nitpick comments:
In `@server/internal/orchestrator/swarm/rag_service_user_role_test.go`:
- Around line 85-120: The test currently only asserts resource ordering/types
(result.Resources) but must also deserialize and assert RAG-specific payload
fields: for the ServiceInstanceSpecResource (use symbol
ServiceInstanceSpecResource or the type used in the diff) deserialize its
payload and assert fields like DatabaseNetworkID, DataDirID, and any
RAG-specific spec values are present and correct; for the
ServiceInstanceResource (symbol ServiceInstanceResource) deserialize and assert
ServiceType == "rag" and other service-level fields introduced by this PR;
likewise deserialize any RAG config/keys resources (ResourceTypeRAGConfig,
ResourceTypeRAGServiceKeys) and assert expected config values and key presence;
update the checks around result.Resources[1], [5], and [6] (and the other ranges
noted) to include these payload assertions rather than relying only on
index/type.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eabcc6a5-7af3-48f8-bd6c-a3200d7191d6
📒 Files selected for processing (7)
server/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/rag_service_keys_resource_test.goserver/internal/orchestrator/swarm/rag_service_user_role_test.goserver/internal/orchestrator/swarm/service_instance.goserver/internal/orchestrator/swarm/service_instance_spec.goserver/internal/orchestrator/swarm/service_spec.goserver/internal/workflows/plan_update.go
✅ Files skipped from review due to trivial changes (2)
- server/internal/workflows/plan_update.go
- server/internal/orchestrator/swarm/service_spec.go
🚧 Files skipped from review as they are similar to previous changes (2)
- server/internal/orchestrator/swarm/rag_service_keys_resource_test.go
- server/internal/orchestrator/swarm/service_instance.go
jason-lynch
approved these changes
Apr 10, 2026
| if len(result.Resources) != 4 { | ||
| t.Fatalf("len(Resources) = %d, want 4", len(result.Resources)) | ||
| // Single node: Network + canonical RO + DirResource + Keys + Config + InstanceSpec + ServiceInstance = 7. | ||
| if len(result.Resources) != 7 { |
Member
There was a problem hiding this comment.
Non-blocking suggestion: I think these tests could be improved by using the same testify library that we use elsewhere. For example, the three lines of this if statement could be replaced with:
require.Len(t, result.Resources, 7)
require.Len's default error message is roughly equivalent to the existing on here, and it doesn't need to be updated when the expected length changes. Side note - Claude always wants to fill in custom error messages for the testify assertions, but IMO they're not more useful than the defaults unless the check is non-intuitive and the message explains why the condition exists.
You might also be able to combine many of these assertions by using assert.Equal/require.Equal, which work great for complex structs, like:
assert.Equal(t, result, &database.ServiceInstanceResources{
// ...
})For me, that would be a lot easier to read and keep up-to-date.
One more suggestion to consider in this file: the file is named server/internal/orchestrator/swarm/rag_service_user_role_test.go but it has tests for other things besides the user role. Should it be renamed?
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/internal/orchestrator/swarm/rag_instance_resources_test.go (1)
54-253: Consider a shared spec fixture builder to reduce repeated setup.
database.ServiceInstanceSpecconstruction is repeated across multiple tests (Line 54, Line 90, Line 144, Line 191, Line 212, Line 239). A small helper would lower maintenance cost when fields evolve.♻️ Example refactor sketch
+func baseRAGInstanceSpec() *database.ServiceInstanceSpec { + return &database.ServiceInstanceSpec{ + ServiceInstanceID: "db1-rag-host1", + ServiceSpec: &database.ServiceSpec{ + ServiceID: "rag", + ServiceType: "rag", + Version: "latest", + Config: minimalRAGConfig(), + }, + DatabaseID: "db1", + DatabaseName: "db1", + HostID: "host-1", + NodeName: "n1", + } +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/swarm/rag_instance_resources_test.go` around lines 54 - 253, Tests duplicate construction of database.ServiceInstanceSpec across TestGenerateRAGInstanceResources*, TestGenerateServiceInstanceResources_*, and TestGenerateRAGInstanceResources_IncompatibleVersion; extract a shared fixture builder function (e.g., newRAGSpec or buildServiceInstanceSpec) to centralize common fields. Implement a helper that accepts overrides (nodeName, hostID, serviceID/version, databaseID, config, DatabaseNodes, PgEdgeVersion) and calls minimalRAGConfig() as default; update tests to call this helper instead of repeating the struct literals (refer to symbols database.ServiceInstanceSpec, minimalRAGConfig, TestGenerateRAGInstanceResources_MultiNode, TestGenerateRAGInstanceResources_MultiNode_CanonicalNotFirst, TestGenerateServiceInstanceResources_RAGDispatch, TestGenerateServiceInstanceResources_UnknownTypeReturnsError, TestGenerateRAGInstanceResources_IncompatibleVersion).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@server/internal/orchestrator/swarm/rag_instance_resources_test.go`:
- Around line 54-253: Tests duplicate construction of
database.ServiceInstanceSpec across TestGenerateRAGInstanceResources*,
TestGenerateServiceInstanceResources_*, and
TestGenerateRAGInstanceResources_IncompatibleVersion; extract a shared fixture
builder function (e.g., newRAGSpec or buildServiceInstanceSpec) to centralize
common fields. Implement a helper that accepts overrides (nodeName, hostID,
serviceID/version, databaseID, config, DatabaseNodes, PgEdgeVersion) and calls
minimalRAGConfig() as default; update tests to call this helper instead of
repeating the struct literals (refer to symbols database.ServiceInstanceSpec,
minimalRAGConfig, TestGenerateRAGInstanceResources_MultiNode,
TestGenerateRAGInstanceResources_MultiNode_CanonicalNotFirst,
TestGenerateServiceInstanceResources_RAGDispatch,
TestGenerateServiceInstanceResources_UnknownTypeReturnsError,
TestGenerateRAGInstanceResources_IncompatibleVersion).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d3b671ff-e47d-4142-85b7-3a7b82a563c3
📒 Files selected for processing (2)
server/internal/orchestrator/swarm/rag_instance_resources_test.goserver/internal/orchestrator/swarm/rag_service_user_role_test.go
💤 Files with no reviewable changes (1)
- server/internal/orchestrator/swarm/rag_service_user_role_test.go
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
2 participants
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
This PR deploys the
pgedge-rag-serveras a Docker Swarm service with the generatedconfig and API key files bind-mounted into the container, completing the
RAG service container deployment layer.
Changes
"rag"case toServiceContainerSpec(): TCP health check,/app/datamount (rw),
/app/keysmount (ro)ServiceTypefield toServiceInstanceResourceand exclude the RW roledependency for RAG in both
ServiceInstanceResourceandServiceInstanceSpecResource(RAG is read-only)"rag"toServiceInstanceSpecResource.Dependencies()soRAGConfigResourceis resolved before the spec is computedNetwork,ServiceInstanceSpecResource, andServiceInstanceResourceinto
generateRAGInstanceResources()completing the full deploy chainplan_update.gonewTestOrchestrator()helper and update resource count assertionsTesting
Verification:
Checklist
Notes for Reviewers
pgedge_application_read_only);the RW role dependency is skipped for
"rag"service type in bothServiceInstanceResourceandServiceInstanceSpecResourcePLAT-492