feat(server): support integration in import cmd #1372
Conversation
WalkthroughThe pull request introduces support for integration-based item imports in the Re:Earth CMS system. The changes modify the command-line import functionality to allow specifying an integration ID alongside the existing user ID. This enhancement provides more flexibility in how import operations are initiated, enabling imports to be performed using either user or integration credentials. The modifications span multiple files, updating the import process's logic to handle integration-based operators and validate payloads accordingly. Changes
Sequence DiagramsequenceDiagram
participant CLI as Command Line Interface
participant MainFunc as Main Function
participant OperatorGen as Operator Generator
participant ImportInteractor as Import Interactor
participant TaskRunner as Task Runner
CLI->>MainFunc: Provide userId or integrationId
alt User-based Import
MainFunc->>OperatorGen: generateUserOperator()
else Integration-based Import
MainFunc->>OperatorGen: generateIntegrationOperator()
end
OperatorGen-->>MainFunc: Return Operator
MainFunc->>ImportInteractor: Initiate Import
ImportInteractor->>TaskRunner: Create Import Task
TaskRunner->>TaskRunner: Execute Import
Possibly related PRs
Poem
✨ Finishing Touches
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 (
|
✅ Deploy Preview for reearth-cms canceled.
|
# Conflicts: # server/cmd/reearth-cms/main.go # server/internal/infrastructure/gcp/taskrunner.go # server/internal/usecase/interactor/item.go # server/pkg/task/task.go
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/cmd/reearth-cms/main.go(5 hunks)server/internal/infrastructure/gcp/taskrunner.go(1 hunks)server/internal/usecase/interactor/item.go(1 hunks)server/pkg/task/task.go(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: ci
server/cmd/reearth-cms/main.go
[error] 350-350: Type mismatch error: cannot use &iId (value of type *idx.ID[accountdomain.Integration]) as *idx.ID[id.Integration] value in struct literal
🔇 Additional comments (9)
server/cmd/reearth-cms/main.go (3)
43-43: Flag naming convention looks good.The new
-integrationIdflag is consistently named and clearly indicates its purpose in the CLI interface.
95-101: Ensure mutual exclusivity or fallback logic is well-defined.Currently, the code checks whether
uIdStroriIdStris provided and picks the first non-empty one. Consider clarifying in the help text or validation logic if both flags are provided at the same time or if only one is expected.
281-285: Refined error handling for string-based user ID conversion.Good approach switching to
stringfor the user ID input. The error return is appropriate if parsing fails, ensuring invalid user IDs do not propagate further.server/pkg/task/task.go (2)
80-80: Field reflects expanded use case.The new
IntegrationIdfield correctly extends the payload structure to support integration-based imports. No immediate issues detected.
90-90: Validation logic updated for flexible identity.Allowing either
UserIdorIntegrationIdremoves the strict reliance on user-based imports. The logical OR condition is concise and clear.server/internal/infrastructure/gcp/taskrunner.go (1)
236-236: Expanded CLI args.Adding
-integrationIdsupports the new integration-based import. Make sure the build steps handle both user and integration IDs as intended.server/internal/usecase/interactor/item.go (3)
637-639: LGTM! Early operator validation is properly implemented.The validation ensures that either a user or integration is present before proceeding with the import operation.
654-659: LGTM! Task payload correctly handles both user and integration scenarios.The implementation properly sets either UserId or IntegrationId in the task payload based on the operator type, which aligns with the PR objective to support integration-based imports.
654-659: Verify task runner compatibility with IntegrationId.Let's ensure the task runner implementation can handle the new IntegrationId field in the payload.
✅ Verification successful
Task runner properly handles IntegrationId field
The task system is correctly implemented to handle the IntegrationId field:
- IntegrationId is properly defined in ImportPayload struct
- Validation logic explicitly accepts either UserId or IntegrationId
- The implementation confirms full compatibility with integration-based imports
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if the task runner implementation handles IntegrationId # Search for IntegrationId handling in task runner code rg -A 5 'IntegrationId.*string' server/pkg/task/Length of output: 344
Script:
#!/bin/bash # Search for task processing logic that uses IntegrationId rg -A 10 'func.*Process.*Task' server/pkg/task/Length of output: 49
Script:
#!/bin/bash # Search for task-related code with broader patterns rg -l "task" server/pkg/task/ && \ echo "=== Task Handler Contents ===" && \ cat server/pkg/task/task.goLength of output: 2015
server/cmd/reearth-cms/main.go
Outdated
| func generateIntegrationOperator(ctx context.Context, iIdStr string, repo *repo.Container, accRepo *accountrepo.Container) (*usecase.Operator, error) { | ||
| iId, err := accountdomain.IntegrationIDFrom(iIdStr) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| w, err := accRepo.Workspace.FindByIntegration(ctx, iId) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| rw := w.FilterByIntegrationRole(iId, workspace.RoleReader).IDs() | ||
| ww := w.FilterByIntegrationRole(iId, workspace.RoleWriter).IDs() | ||
| mw := w.FilterByIntegrationRole(iId, workspace.RoleMaintainer).IDs() | ||
| ow := w.FilterByIntegrationRole(iId, workspace.RoleOwner).IDs() | ||
|
|
||
| rp, wp, mp, op, err := operatorProjects(ctx, repo, w, rw, ww, mw, ow) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return &usecase.Operator{ | ||
| AcOperator: &accountusecase.Operator{ | ||
| User: nil, | ||
| ReadableWorkspaces: rw, | ||
| WritableWorkspaces: ww, | ||
| MaintainableWorkspaces: mw, | ||
| OwningWorkspaces: ow, | ||
| }, | ||
| Integration: &iId, | ||
| Lang: language.English.String(), | ||
| ReadableProjects: rp, | ||
| WritableProjects: wp, | ||
| MaintainableProjects: mp, | ||
| OwningProjects: op, | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
Fix type mismatch for the Integration field.
The pipeline error indicates a mismatch between accountdomain.IntegrationID and id.IntegrationID. Update the code to properly convert the parsed integration ID to match the type expected by usecase.Operator. For example:
- Integration: &iId,
+ // Convert accountdomain.IntegrationID (iId) to the correct id.IntegrationID.
+ integrationId2, err := id.IntegrationIDFrom(iId.String())
+ if err != nil {
+ return nil, err
+ }
+ Integration: &integrationId2,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func generateIntegrationOperator(ctx context.Context, iIdStr string, repo *repo.Container, accRepo *accountrepo.Container) (*usecase.Operator, error) { | |
| iId, err := accountdomain.IntegrationIDFrom(iIdStr) | |
| if err != nil { | |
| return nil, err | |
| } | |
| w, err := accRepo.Workspace.FindByIntegration(ctx, iId) | |
| if err != nil { | |
| return nil, err | |
| } | |
| rw := w.FilterByIntegrationRole(iId, workspace.RoleReader).IDs() | |
| ww := w.FilterByIntegrationRole(iId, workspace.RoleWriter).IDs() | |
| mw := w.FilterByIntegrationRole(iId, workspace.RoleMaintainer).IDs() | |
| ow := w.FilterByIntegrationRole(iId, workspace.RoleOwner).IDs() | |
| rp, wp, mp, op, err := operatorProjects(ctx, repo, w, rw, ww, mw, ow) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return &usecase.Operator{ | |
| AcOperator: &accountusecase.Operator{ | |
| User: nil, | |
| ReadableWorkspaces: rw, | |
| WritableWorkspaces: ww, | |
| MaintainableWorkspaces: mw, | |
| OwningWorkspaces: ow, | |
| }, | |
| Integration: &iId, | |
| Lang: language.English.String(), | |
| ReadableProjects: rp, | |
| WritableProjects: wp, | |
| MaintainableProjects: mp, | |
| OwningProjects: op, | |
| }, nil | |
| } | |
| func generateIntegrationOperator(ctx context.Context, iIdStr string, repo *repo.Container, accRepo *accountrepo.Container) (*usecase.Operator, error) { | |
| iId, err := accountdomain.IntegrationIDFrom(iIdStr) | |
| if err != nil { | |
| return nil, err | |
| } | |
| w, err := accRepo.Workspace.FindByIntegration(ctx, iId) | |
| if err != nil { | |
| return nil, err | |
| } | |
| rw := w.FilterByIntegrationRole(iId, workspace.RoleReader).IDs() | |
| ww := w.FilterByIntegrationRole(iId, workspace.RoleWriter).IDs() | |
| mw := w.FilterByIntegrationRole(iId, workspace.RoleMaintainer).IDs() | |
| ow := w.FilterByIntegrationRole(iId, workspace.RoleOwner).IDs() | |
| rp, wp, mp, op, err := operatorProjects(ctx, repo, w, rw, ww, mw, ow) | |
| if err != nil { | |
| return nil, err | |
| } | |
| integrationId2, err := id.IntegrationIDFrom(iId.String()) | |
| if err != nil { | |
| return nil, err | |
| } | |
| return &usecase.Operator{ | |
| AcOperator: &accountusecase.Operator{ | |
| User: nil, | |
| ReadableWorkspaces: rw, | |
| WritableWorkspaces: ww, | |
| MaintainableWorkspaces: mw, | |
| OwningWorkspaces: ow, | |
| }, | |
| Integration: &integrationId2, | |
| Lang: language.English.String(), | |
| ReadableProjects: rp, | |
| WritableProjects: wp, | |
| MaintainableProjects: mp, | |
| OwningProjects: op, | |
| }, nil | |
| } |
🧰 Tools
🪛 GitHub Actions: ci
[error] 350-350: Type mismatch error: cannot use &iId (value of type *idx.ID[accountdomain.Integration]) as *idx.ID[id.Integration] value in struct literal
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
server/cmd/reearth-cms/main.go (2)
96-102: Enhance error handling specificity.While the logic for operator generation is correct, the error message could be more specific about which operation failed (user-based or integration-based).
Consider this improvement:
- if err != nil || op == nil { - log.Fatalf("failed to generate operator: %v", err) + if err != nil || op == nil { + source := "user" + if iIdStr != nil && *iIdStr != "" { + source = "integration" + } + log.Fatalf("failed to generate %s-based operator: %v", source, err)
323-358: Add function documentation.Consider adding documentation to explain the purpose and behavior of this function, especially regarding workspace permissions and integration-based access control.
Add this documentation:
+// generateIntegrationOperator creates a new operator for integration-based operations. +// It retrieves workspace permissions for the given integration ID and sets up appropriate +// access levels for projects within those workspaces. func generateIntegrationOperator(ctx context.Context, iIdStr string, repo *repo.Container, accRepo *accountrepo.Container) (*usecase.Operator, error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/cmd/reearth-cms/main.go(5 hunks)
🔇 Additional comments (2)
server/cmd/reearth-cms/main.go (2)
17-17: LGTM: Import and flag definitions are correct.The new import and flag definitions are properly implemented to support integration-based imports.
Also applies to: 44-44
351-351: Verify integration ID type conversion.The integration ID conversion needs verification to ensure type compatibility between
accountdomain.IntegrationIDandintegration2.ID.Run this script to verify the type conversion:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor