chore: clean stack v1 support (EN-471) #95
Conversation
WalkthroughMigrate orchestration API calls from V1 to V2 across instances, triggers, and workflows, update request/response types and mappings to V2 shapes, add nil-safety checks for nested V2 fields, and remove the search subcommand plus its view helpers from the CLI. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI Controller
participant SDK as Orchestration.V2 SDK
participant API as Orchestration Service
rect `#e6f7ff`
CLI->>SDK: V2GetInstanceHistoryRequest(instanceID)
SDK->>API: HTTP V2 /instances/{id}/history
API-->>SDK: V2GetWorkflowInstanceHistoryResponse{ Data: V2WorkflowInstanceHistory[] }
SDK-->>CLI: Response (V2 types)
end
rect `#fff7e6`
CLI->>CLI: map/convert V2 types -> internal display structs (with nil checks)
CLI-->>User: render history/stages
end
Note: similar flow applies for RunWorkflow/GetWorkflow, ListTriggers/ListWorkflows, and other migrated endpoints—requests now target Orchestration.V2 and return V2 response shapes which the CLI maps to internal representations before rendering. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
cmd/orchestration/triggers/occurrences/list.go (1)
77-82: Fix inverted nil check logic.The nil check is backwards. When
WorkflowInstanceIDis not nil, the code returns an empty string. When it is nil, the code attempts to dereference it, which will cause a panic.Apply this diff to fix the logic:
return []string{ func() string { - if src.WorkflowInstanceID != nil { - return "" - } - return *src.WorkflowInstanceID + if src.WorkflowInstanceID == nil { + return "" + } + return *src.WorkflowInstanceID }(),
🧹 Nitpick comments (2)
cmd/orchestration/workflows/run.go (1)
99-114: Workflow conversion forPrintWorkflowInstancelooks consistent, but confirm V2 model shapeConverting the V2 workflow into a
shared.Workflowbefore passing it tointernal.PrintWorkflowInstancematches the pattern introduced ininstances/show.goand keeps printing isolated from V2‑specific types.Please verify against the SDK that:
operations.V2GetWorkflowRequestusesFlowID(notWorkflowIDor another field name).w.V2GetWorkflowResponse.DataexposesID,CreatedAt,UpdatedAt, andConfigwith types compatible withshared.Workflowandshared.WorkflowConfig.If those align, this block is good and keeps the rendering logic consistent across commands.
cmd/orchestration/triggers/create.go (1)
67-75: Create‑trigger V2 payload and mapping are coherent; verifyVarsand response typesThe V2 migration here hangs together well:
shared.V2TriggerDatais populated withEvent,Name,WorkflowID, an initially emptyVarsmap, and an optionalFilter.--varsare parsed askey=valuepairs and added todata.Vars.store.Client().Orchestration.V2.CreateTrigger(ctx, data)is used, and the resulting V2 trigger is mapped intoshared.Triggerwith all fields that other commands expect.Two minor follow‑ups:
- Confirm in the SDK that
VarsonV2TriggerDataismap[string]interface{}(or compatible withmap[string]interface{}) so that assigningmap[string]interface{}{}andstringvalues is correct.- Confirm that
res.V2CreateTriggerResponse.Datais the right field path and that its fields (ID,Name,WorkflowID,Event,Filter,Vars,CreatedAt) map directly toshared.Trigger.Also, the error message
"reading trigger"on create is slightly misleading; if you touch this again, consider something like"creating trigger"for clarity, but it’s not blocking.Also applies to: 76-84, 86-101
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (22)
cmd/orchestration/instances/describe.go(9 hunks)cmd/orchestration/instances/list.go(1 hunks)cmd/orchestration/instances/send_event.go(1 hunks)cmd/orchestration/instances/show.go(3 hunks)cmd/orchestration/instances/stop.go(1 hunks)cmd/orchestration/triggers/create.go(2 hunks)cmd/orchestration/triggers/delete.go(1 hunks)cmd/orchestration/triggers/list.go(1 hunks)cmd/orchestration/triggers/occurrences/list.go(3 hunks)cmd/orchestration/triggers/show.go(1 hunks)cmd/orchestration/workflows/create.go(1 hunks)cmd/orchestration/workflows/delete.go(1 hunks)cmd/orchestration/workflows/list.go(2 hunks)cmd/orchestration/workflows/run.go(2 hunks)cmd/orchestration/workflows/show.go(1 hunks)cmd/payments/versions/versions.go(1 hunks)cmd/root.go(0 hunks)cmd/search/root.go(0 hunks)cmd/search/views/accounts.go(0 hunks)cmd/search/views/assets.go(0 hunks)cmd/search/views/payment.go(0 hunks)cmd/search/views/transactions.go(0 hunks)
💤 Files with no reviewable changes (6)
- cmd/search/views/transactions.go
- cmd/root.go
- cmd/search/views/assets.go
- cmd/search/views/payment.go
- cmd/search/root.go
- cmd/search/views/accounts.go
🧰 Additional context used
🧬 Code graph analysis (6)
cmd/orchestration/triggers/list.go (1)
pkg/utils.go (1)
Map(20-26)
cmd/orchestration/workflows/run.go (2)
cmd/orchestration/instances/list.go (1)
WorkflowInstance(14-20)cmd/orchestration/workflows/list.go (1)
Workflow(14-19)
cmd/orchestration/workflows/list.go (1)
pkg/utils.go (1)
Map(20-26)
cmd/orchestration/instances/show.go (1)
pkg/utils.go (1)
Map(20-26)
cmd/orchestration/workflows/show.go (1)
cmd/orchestration/workflows/list.go (1)
Workflow(14-19)
cmd/orchestration/instances/list.go (2)
pkg/flags.go (3)
Ptr(89-91)GetBool(26-33)GetString(35-41)pkg/utils.go (1)
Map(20-26)
🔇 Additional comments (20)
cmd/payments/versions/versions.go (1)
26-26: Verify V1 API usage aligns with PR objectives.Line 26 calls
store.Client().Payments.V1.PaymentsgetServerInfo(), which is still using the V1 API endpoint. Given that this PR's objective is to "clean stack v1 support" and migrate orchestration calls to V2, please confirm whether this call should also be migrated to a V2 endpoint to maintain consistency across the codebase.cmd/orchestration/instances/send_event.go (1)
50-55: Migration to V2 API verified and approved.The V2 SendEvent API matches the code implementation: the request correctly passes the event name and instance ID, and appropriately discards the 204 No Content response. Field mapping is accurate and error handling is in place. The code is ready.
cmd/orchestration/instances/stop.go (1)
46-48: V1→V2 migration verified complete—no action needed.The verification confirms that the V1→V2 migration in
cmd/orchestration/instances/stop.go(lines 46-48) is complete and correct. No remainingOrchestration.V1API calls were found in the codebase, indicating consistent migration across all orchestration commands.cmd/orchestration/triggers/delete.go (1)
46-48: V2 API migration is correctly implemented.Verification confirms the changes are appropriate:
V2DeleteTriggerRequeststructure is consistent with the V1 version and other orchestration operations- V2
DeleteTriggerreturns a void response (no response type defined), so discarding it with_is correct- Error handling is unchanged and compatible with V2
- Pattern is consistent with other migrated operations (e.g.,
DeleteWorkflow)- V1 support has been fully removed from orchestration code
cmd/orchestration/workflows/run.go (1)
70-75: Check V2 RunWorkflow request and instance mapping against SDK typesThe overall flow (building
variables, passingWait, then mapping the V2 response intoshared.WorkflowInstance) is consistent with how the rest of the PR handles V2 → shared conversions.Two things are worth double‑checking:
- That
operations.V2RunWorkflowRequestreally expectsRequestBodyas the same type you’re passing (map[string]stringhere) andWaitas*bool. A mismatch (e.g.,map[string]interface{}) would be a compile‑time issue.- That all the fields you map from
v2Instance(ID,WorkflowID,CreatedAt,UpdatedAt,Terminated,TerminatedAt,Error) exist on the V2 instance type and line up 1:1 withshared.WorkflowInstance.Assuming those match the SDK, the conversion looks structurally sound and keeps the existing
PrintWorkflowInstancecontract.Also applies to: 81-91
cmd/orchestration/instances/show.go (1)
51-56: V2 GetInstance/GetWorkflow usage and workflow mapping mostly look fine; verify SDK field namesThe switch to:
Orchestration.V2.GetInstance(..., operations.V2GetInstanceRequest{ InstanceID: args[0] })andOrchestration.V2.GetWorkflow(..., operations.V2GetWorkflowRequest{ FlowID: res.V2GetWorkflowInstanceResponse.Data.WorkflowID })together with the
shared.Workflowmapping fromresponse.V2GetWorkflowResponse.Datakeeps the old responsibilities but on V2 models.Please double‑check the following against the SDK:
- That the request struct fields are indeed
InstanceIDandFlowID.- That the response wrappers
V2GetWorkflowInstanceResponseandV2GetWorkflowResponseand theirDatafields match what you’re using.- That
v2Workflow.Configcan be safely cast toshared.WorkflowConfigas done here.If those all line up, the run path and workflow mapping look correct.
Also applies to: 58-73
cmd/orchestration/triggers/list.go (1)
53-55: V2 ListTriggers integration and mapping look consistent; just confirm SDK shapesThe migration to:
Orchestration.V2.ListTriggers(..., operations.V2ListTriggersRequest{ Name: &name }), and- mapping
response.V2ListTriggersResponse.Cursor.Data(V2 triggers) into[]shared.Triggerfits the overall V2 pattern in this PR, and the mapping copies all the fields that
Renderrelies on (ID,Name,WorkflowID,Event,Filter,Vars,CreatedAt).Please verify against the SDK that:
operations.V2ListTriggersRequesttakesName *string.- The response exposes
V2ListTriggersResponse.Cursor.Datawith elements of typeshared.V2Triggerhaving the fields you’re mapping.shared.Triggerhas matching types for these fields (especiallyNameandFilterpointers andVarsmap type).If that’s all aligned, the list behavior should be unchanged except for now using the V2 API.
Also applies to: 61-73
cmd/orchestration/triggers/show.go (1)
49-54: Trigger V2 read + mapping is consistent with list/create; verify API contractThis block cleanly switches the “show” path to V2 and reuses the same V2Trigger → shared.Trigger mapping pattern as
triggers/list.goandtriggers/create.go, which is good for consistency.To be safe, please confirm:
operations.V2ReadTriggerRequestindeed usesTriggerIDas the field name.res.V2ReadTriggerResponse.Datais the correct path for the trigger payload.- The types of
Name,Filter,Vars, andCreatedAton the V2 model line up withshared.Triggerso that later dereferences (*c.store.Trigger.Name) and formatting don’t panic.Assuming those contracts hold, this change is straightforward.
Also applies to: 56-66
cmd/orchestration/workflows/list.go (1)
57-60: V2 ListWorkflows migration is clean; double‑check response path and config name handlingSwitching to
Orchestration.V2.ListWorkflowswith an emptyoperations.V2ListWorkflowsRequest{}and then mappingresponse.V2ListWorkflowsResponse.Cursor.Datainto your localWorkflowstruct preserves the existing table UX:
- ID is passed through.
- Name is pulled from
src.Config.Namewith a nil‑safe fallback to"".CreatedAtandUpdatedAtare formatted as RFC3339 strings.Please verify with the SDK that:
V2ListWorkflowsRequestreally takes no required fields in this usage.- The response type exposes
V2ListWorkflowsResponse.Cursor.Datawith elements of typeshared.V2WorkflowwithID,Config.Name,CreatedAt, andUpdatedAtas used here.Given that, this migration looks good and consistent with the rest of the V2 changes.
Also applies to: 62-74
cmd/orchestration/instances/list.go (1)
65-68: V2 ListInstances wiring and mapping look good; confirm response wrapper namingThis function now:
- Builds
operations.V2ListInstancesRequestwithRunningandWorkflowIDpopulated from flags and wrapped in pointers, and- Maps
response.V2ListRunsResponse.Cursor.Data(V2 workflow instances) into a local, render‑friendlyWorkflowInstancestruct, carefully handling a possibly nilTerminatedAtby returning""in that case.The mapping matches the columns rendered below and safely avoids nil time dereferences.
The one thing to verify is the SDK contract:
- That
V2ListInstancesRequestexposesRunning *boolandWorkflowID *stringas used.- That the response type’s field really is
V2ListRunsResponse.Cursor.Data(the “Runs” vs “Instances” naming is a bit surprising) and that those elements haveID,WorkflowID,CreatedAt,UpdatedAt, andTerminatedAt *time.Time.Assuming that’s accurate, this is a solid migration.
Also applies to: 73-86
cmd/orchestration/workflows/delete.go (1)
45-50: LGTM! Clean V2 migration.The migration from V1 to V2 API is straightforward and correct. The request type and endpoint are properly updated while maintaining the FlowID parameter.
cmd/orchestration/triggers/occurrences/list.go (1)
16-16: LGTM! Proper V2 migration.The type updates and API call migration to V2 are correct. The response path properly reflects the new cursor-based pagination structure.
Also applies to: 51-58
cmd/orchestration/instances/describe.go (6)
17-17: LGTM! Comprehensive V2 type migration.The migration properly updates all types from V1 to V2, including the store field, API call, and all helper function signatures. The V2 subject type handling is correctly updated to use
V2WalletSubjectandV2LedgerAccountSubject.Also applies to: 52-59, 75-75, 83-83, 96-96, 109-118, 132-132
139-156: LGTM! Proper nil checks added for V2 stage handling.The V2 stage type migration includes appropriate nil checks before accessing
V2StageSendfields, preventing potential panics. The API call toV2.GetInstanceStageHistoryis correctly updated.
164-187: LGTM! Thorough defensive nil checks.The nested nil checks for
CreateTransactiondata, postings, output, and metadata are well-structured and prevent potential panics when accessing V2 response data. The logic correctly validates array lengths before indexing.
248-269: LGTM! Proper V2 stage handling with nil checks.The delay and wait event stages are correctly migrated to V2 types with appropriate nil checks. The delay stage properly handles both
DurationandUntilvariants.
270-279: LGTM! V2 update stage properly handled.The update stage migration includes proper nil checks and correctly handles account metadata updates. The V2 type usage is appropriate.
231-231: Clarify if Orchestration RevertTransaction removal is a documented breaking change.This comment appears within a deliberate V1→V2 API migration (commit: "chore: clean stack v1 support"). RevertTransaction still exists in the Ledger V2 API, so this change is specific to the Orchestration workflow instance history rendering—not a product feature removal.
Before flagging as a breaking change, confirm:
- Whether this Orchestration API incompatibility is documented as a known breaking change
- Whether V1 workflows with RevertTransaction stages fail when executed against V2
- If users need migration guidance for affected workflows
cmd/orchestration/workflows/show.go (1)
59-66: Verify type conversion safety by checking the formance-sdk-go SDK package.The direct type conversion on line 65 (
shared.WorkflowConfig(v2Workflow.Config)) cannot be definitively verified within this repository, as bothshared.WorkflowConfigand the Config type from the V2 API response are defined in the external SDK packagegithub.com/formancehq/formance-sdk-go/v3.To confirm this conversion is safe and won't lose data:
- Check the
formance-sdk-gorepository to compare the field definitions ofWorkflowConfigand the V2 Config type- Verify they have identical or compatible fields
- Ensure no fields present in the V2 response type are missing from
WorkflowConfigThe same conversion pattern appears in
cmd/orchestration/workflows/run.go(line 112), suggesting this is intentional, but the actual type safety depends on the SDK's type definitions.cmd/orchestration/workflows/create.go (1)
52-62: Verify all WorkflowConfig fields are mapped to V2WorkflowConfig.The code unmarshals YAML into
WorkflowConfigand maps onlyNameandStagesto the V2 API call. This concern cannot be automatically verified because the struct definitions are part of the externalformance-sdk-go/v3SDK. Ensure that:
- No additional fields from
WorkflowConfigare being silently dropped during the V2 migration- The field mapping is intentional and aligns with API requirements
Review the SDK types in
github.com/formancehq/formance-sdk-go/v3/pkg/models/sharedand confirm the migration is complete.
| // Convert V2WorkflowInstance to WorkflowInstance | ||
| v2Instance := c.store.WorkflowInstance | ||
| instance := shared.WorkflowInstance{ | ||
| ID: v2Instance.ID, | ||
| WorkflowID: v2Instance.WorkflowID, | ||
| CreatedAt: v2Instance.CreatedAt, | ||
| UpdatedAt: v2Instance.UpdatedAt, | ||
| Terminated: v2Instance.Terminated, | ||
| TerminatedAt: v2Instance.TerminatedAt, | ||
| Error: v2Instance.Error, | ||
| Status: fctl.Map(v2Instance.Status, func(src shared.V2StageStatus) shared.StageStatus { | ||
| return shared.StageStatus{ | ||
| StartedAt: src.StartedAt, | ||
| TerminatedAt: src.TerminatedAt, | ||
| Error: src.Error, | ||
| } | ||
| }), | ||
| } | ||
| if err := internal.PrintWorkflowInstance(cmd.OutOrStdout(), c.store.Workflow, instance); err != nil { |
There was a problem hiding this comment.
🧩 Analysis chain
Status conversion from V2 → shared instance is clear; consider defensive checks on time/status fields
The conversion from shared.V2WorkflowInstance to shared.WorkflowInstance, including:
- copying the scalar fields and timestamps, and
- mapping
[]shared.V2StageStatus→[]shared.StageStatusviafctl.Map
is clean and keeps internal.PrintWorkflowInstance on stable, non‑V2 types.
Two minor points:
TerminatedAt.Format(time.RFC3339)above in the info table assumesTerminatedAtis non‑nil wheneverTerminatedis true. If the API ever returnsTerminatedAt=nilfor a terminated instance, that would panic. A nil‑check before formatting would make this more robust.- Likewise, if any
StartedAt/TerminatedAtinsrc shared.V2StageStatuscan be nil pointers (or zero values where the type allows it), you may want to mirror the list‑command pattern and guard those before formatting/printing downstream.
Behavior‑wise, the mapping approach is solid; this is mostly about hardening against unexpected API responses.
---
🏁 Script executed:
```shell
#!/bin/bash
# First, find and examine the complete show.go file
fd -t f "show.go" --path "*orchestration/instances*"
Length of output: 233
🏁 Script executed:
#!/bin/bash
# Search for the earlier TerminatedAt.Format call mentioned in the review
rg -n "TerminatedAt.*Format" --type goLength of output: 468
🏁 Script executed:
#!/bin/bash
# Search for PrintWorkflowInstance function to understand how it handles nil values
rg -n -A 20 "func.*PrintWorkflowInstance" --type goLength of output: 1413
🏁 Script executed:
#!/bin/bash
# Find the list command implementation to understand the defensive pattern mentioned
fd -t f "*.go" --path "*orchestration*" -x rg -l "list.*command\|List.*Command" {} \;Length of output: 233
🏁 Script executed:
#!/bin/bash
# Search for WorkflowInstance type definitions to understand field types
rg -n "type.*WorkflowInstance\|type.*V2WorkflowInstance" --type go -A 15Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Get full show.go file to see complete context
find . -name "show.go" -path "*orchestration/instances*" -type f
Length of output: 37
🏁 Script executed:
#!/bin/bash
# Show full context around line 86 and 99-117 in show.go
rg -n -B 5 -A 15 "TerminatedAt.Format.*show.go" --type go || rg -n -B 5 -A 15 "Terminated at" cmd/orchestration/instances/show.go --type goLength of output: 1076
🏁 Script executed:
#!/bin/bash
# Get full context of show.go file around lines 80-120
sed -n '80,120p' cmd/orchestration/instances/show.goLength of output: 1660
🏁 Script executed:
#!/bin/bash
# Search for type definitions in shared package
rg -n "^type (WorkflowInstance|V2WorkflowInstance|StageStatus|V2StageStatus)" --type go -A 12Length of output: 916
🏁 Script executed:
#!/bin/bash
# Check if there's nil-guarding pattern for StartedAt elsewhere
rg -n "StartedAt.*nil\|nil.*StartedAt" --type go -B 2 -A 2Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Check imports and beginning of show.go to find shared package location
head -30 cmd/orchestration/instances/show.go
Length of output: 839
🏁 Script executed:
#!/bin/bash
# Search for shared.WorkflowInstance type definition more broadly
rg -n "type WorkflowInstance\|type V2WorkflowInstance\|type StageStatus\|type V2StageStatus" --type go -A 15Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for shared package definitions in parent directories
find . -name "*.go" -type f | xargs rg -l "type WorkflowInstance" | head -20Length of output: 37
🏁 Script executed:
#!/bin/bash
# Get full print.go to see complete PrintWorkflowInstance implementation
cat cmd/orchestration/internal/print.goLength of output: 1179
🏁 Script executed:
#!/bin/bash
# Check the V2StageStatus and StageStatus types specifically
rg -n "V2StageStatus\|type StageStatus" --type go -B 1 -A 8Length of output: 0
---
🏁 Script executed:
```shell
#!/bin/bash
# Search list.go for the defensive pattern with StartedAt/TerminatedAt
cat cmd/orchestration/instances/list.go | grep -n -A 20 "StartedAt\|TerminatedAt" | head -60
Length of output: 1532
🏁 Script executed:
#!/bin/bash
# Check if StartedAt appears with nil checks anywhere in the codebase
rg -n "StartedAt" --type go -B 2 -A 2 | head -50Length of output: 1454
🏁 Script executed:
#!/bin/bash
# Look for patterns where fields from SDK might be pointers or nullable
rg -n "\*time.Time\|NullTime\|Optional" cmd/orchestration/ --type go | head -20Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check the triggers/occurrences/list.go since it also formats TerminatedAt
rg -n -B 5 -A 5 "TerminatedAt.Format" cmd/orchestration/triggers/occurrences/list.goLength of output: 478
StartedAt requires nil-guard in PrintWorkflowInstance before formatting
The concern about nil-safety is valid but applies to a different line than stated. Line 86 in show.go already guards TerminatedAt with if c.store.WorkflowInstance.Terminated { ... }.
However, in cmd/orchestration/internal/print.go at line 31, src.StartedAt.Format(time.RFC3339) is called without a nil check, while line 33 properly guards TerminatedAt with if src.TerminatedAt != nil { ... }. This inconsistency means if the API returns a nil StartedAt, line 31 will panic. Apply the same defensive pattern used for TerminatedAt (lines 33–34) to StartedAt at line 31.
🤖 Prompt for AI Agents
In cmd/orchestration/internal/print.go around line 31, the code calls
src.StartedAt.Format(time.RFC3339) without guarding against nil which can panic;
mirror the defensive pattern used for TerminatedAt (lines 33–34) by checking if
src.StartedAt != nil before calling Format, set the formatted value only inside
that branch (and leave it empty or nil otherwise) so StartedAt is nil-safe when
printing.
cmd/payments/versions/versions.go
Outdated
|
|
||
| version := "v" + *response.PaymentsServerInfo.Version | ||
|
|
||
There was a problem hiding this comment.
Remove unnecessary blank line.
The extra blank line after the version assignment appears to be unintended whitespace and doesn't align with the PR objectives. Please remove it.
version := "v" + *response.PaymentsServerInfo.Version
-
if !semver.IsValid(version) {📝 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.
| version := "v" + *response.PaymentsServerInfo.Version | |
| if !semver.IsValid(version) { |
🤖 Prompt for AI Agents
In cmd/payments/versions/versions.go around line 36, there is an unnecessary
blank line immediately following the version assignment; remove that extra empty
line so the file has no unintended whitespace at that location and keeps the
surrounding code compact and aligned with the PR style.
gfyrag
force-pushed
the
chore/clean-v1-support
branch
from
f78567d to
1ad642c
Compare
gfyrag
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/orchestration/triggers/occurrences/list.go (1)
78-83: Critical logic error: inverted nil check will cause panic.The conditional logic is inverted. When
src.WorkflowInstanceIDisnil, the code attempts to dereference it (line 82), which will cause a panic. When it's notnil, an empty string is returned instead of the actual value.Apply this diff to fix the inverted logic:
func() string { - if src.WorkflowInstanceID != nil { + if src.WorkflowInstanceID == nil { return "" } return *src.WorkflowInstanceID }(),
🧹 Nitpick comments (3)
cmd/orchestration/workflows/create.go (1)
54-69: Consider unmarshalling YAML directly intoV2WorkflowConfigYou currently unmarshal into
shared.WorkflowConfigand then copyName/Stagesinto ashared.V2WorkflowConfigfor the V2 call. That duplicates config representations and risks missing new V2-only fields.You could simplify and keep the CLI in sync with the V2 schema by unmarshalling straight into the V2 type and passing it through:
- config := shared.WorkflowConfig{} - if err := yaml.Unmarshal([]byte(script), &config); err != nil { - return nil, err - } - - //nolint:gosimple - response, err := store.Client().Orchestration.V2. - CreateWorkflow(cmd.Context(), &shared.V2WorkflowConfig{ - Name: config.Name, - Stages: config.Stages, - }) + config := shared.V2WorkflowConfig{} + if err := yaml.Unmarshal([]byte(script), &config); err != nil { + return nil, err + } + + response, err := store.Client().Orchestration.V2. + CreateWorkflow(cmd.Context(), &config)cmd/orchestration/instances/show.go (1)
53-75: V2 GetInstance/GetWorkflow usage and workflow conversion are consistent; consider a shared helperThe switch to
Orchestration.V2.GetInstanceandOrchestration.V2.GetWorkflow, plus the explicitV2Workflow → shared.Workflowconversion (ID,CreatedAt,UpdatedAt,Config) looks correct and keeps the printer decoupled from V2 types.The same conversion pattern appears in
cmd/orchestration/workflows/run.go. To avoid drift, you might extract a small helper (e.g. incmd/orchestration/internal) like:func FromV2Workflow(v2 shared.V2Workflow) shared.Workflow { return shared.Workflow{ ID: v2.ID, CreatedAt: v2.CreatedAt, UpdatedAt: v2.UpdatedAt, Config: shared.WorkflowConfig(v2.Config), } }and call it from both sites.
cmd/orchestration/workflows/run.go (1)
101-116: Avoid panicking onGetWorkflowerror when--waitis usedIn the wait path, a failure of
Orchestration.V2.GetWorkflowcurrently leads topanic(err), which will dump a stack trace instead of a clean CLI error.Consider returning a wrapped error instead so the top‑level command can handle it consistently:
- w, err := store.Client().Orchestration.V2.GetWorkflow(cmd.Context(), operations.V2GetWorkflowRequest{ + w, err := store.Client().Orchestration.V2.GetWorkflow(cmd.Context(), operations.V2GetWorkflowRequest{ FlowID: args[0], }) if err != nil { - panic(err) + return errors.Wrap(err, "getting workflow") }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (21)
cmd/orchestration/instances/describe.go(9 hunks)cmd/orchestration/instances/list.go(1 hunks)cmd/orchestration/instances/send_event.go(1 hunks)cmd/orchestration/instances/show.go(3 hunks)cmd/orchestration/instances/stop.go(1 hunks)cmd/orchestration/triggers/create.go(2 hunks)cmd/orchestration/triggers/delete.go(1 hunks)cmd/orchestration/triggers/list.go(1 hunks)cmd/orchestration/triggers/occurrences/list.go(3 hunks)cmd/orchestration/triggers/show.go(1 hunks)cmd/orchestration/workflows/create.go(1 hunks)cmd/orchestration/workflows/delete.go(1 hunks)cmd/orchestration/workflows/list.go(2 hunks)cmd/orchestration/workflows/run.go(2 hunks)cmd/orchestration/workflows/show.go(1 hunks)cmd/root.go(0 hunks)cmd/search/root.go(0 hunks)cmd/search/views/accounts.go(0 hunks)cmd/search/views/assets.go(0 hunks)cmd/search/views/payment.go(0 hunks)cmd/search/views/transactions.go(0 hunks)
💤 Files with no reviewable changes (6)
- cmd/root.go
- cmd/search/views/assets.go
- cmd/search/views/payment.go
- cmd/search/views/transactions.go
- cmd/search/root.go
- cmd/search/views/accounts.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/orchestration/triggers/list.go
- cmd/orchestration/triggers/show.go
- cmd/orchestration/workflows/list.go
- cmd/orchestration/instances/stop.go
- cmd/orchestration/workflows/show.go
🧰 Additional context used
🧬 Code graph analysis (3)
cmd/orchestration/workflows/run.go (2)
cmd/orchestration/instances/list.go (1)
WorkflowInstance(16-22)cmd/orchestration/workflows/list.go (1)
Workflow(16-21)
cmd/orchestration/instances/list.go (2)
pkg/flags.go (3)
Ptr(89-91)GetBool(26-33)GetString(35-41)pkg/utils.go (1)
Map(22-28)
cmd/orchestration/instances/show.go (3)
cmd/orchestration/instances/list.go (1)
WorkflowInstance(16-22)cmd/orchestration/workflows/list.go (1)
Workflow(16-21)pkg/utils.go (1)
Map(22-28)
🔇 Additional comments (21)
cmd/orchestration/triggers/create.go (3)
69-86: LGTM!The migration from
shared.TriggerDatatoshared.V2TriggerDatais correct, and all required fields are properly populated. The variable parsing logic remains intact.
88-91: LGTM!The API call correctly migrates from V1 to V2, and error handling is preserved.
93-103: Let me search the codebase directly for the struct definitions:Unable to verify field mapping due to codebase inaccessibility.
The previous verification script returned no results for the struct definitions. To complete the review verification, I would need to:
- Search the codebase for the
V2Triggerstruct definition (likely in an external API client library)- Find the
shared.Triggerstruct definition- Compare field names and types to confirm all fields are properly mapped
Without access to these struct definitions, I cannot verify whether:
- All fields from V2Trigger are being mapped to shared.Trigger
- Field types are compatible
- Any fields are missing from the conversion
cmd/orchestration/triggers/delete.go (1)
48-53: LGTM!The migration from V1 to V2 for the DeleteTrigger API call is straightforward and correct. The request structure and error handling are properly maintained.
cmd/orchestration/triggers/occurrences/list.go (2)
17-17: LGTM!The field type change from
[]shared.TriggerOccurrenceto[]shared.V2TriggerOccurrencealigns with the V2 API migration.
52-59: LGTM!The V2 API migration is correct. The response path now uses
V2ListTriggersOccurrencesResponse.Cursor.Data, which is consistent with cursor-based pagination in V2 APIs.cmd/orchestration/instances/describe.go (8)
18-20: LGTM! Struct field updated for V2 migration.The field type change from V1 to V2 is consistent with the PR objectives and properly used throughout the file.
54-61: LGTM! API call successfully migrated to V2.The endpoint and response handling are correctly updated to use V2 types and paths.
98-109: Verify nil handling for destination parameter.Similar to
stageSourceName, this function accepts a pointer*shared.V2StageSendDestinationbut doesn't check ifdstitself is nil before accessing its fields.Please verify whether
dstcan be nil in the V2 API. If it can, add a nil check:func stageDestinationName(dst *shared.V2StageSendDestination) string { if dst == nil { return "unknown_source_type" } switch { // ... rest of the logic
111-120: LGTM! Subject name function correctly handles V2 types.The function properly checks for nil on the nested pointer fields before accessing them.
134-149: LGTM! Proper nil checks added for V2 stage send.The nil check on line 143 prevents potential panics when accessing nested fields of
V2StageSend.
150-249: LGTM! Comprehensive nil checks throughout stage history handling.The extensive nil and bounds checking (lines 167, 177, 179, 183-186) properly prevents panics when accessing nested V2 response fields. The comment on line 233 appropriately documents the intentional removal of RevertTransaction support in V2.
250-294: LGTM! All stage type handlers include proper nil guards.The nil checks for
V2StageDelay(line 252),V2StageWaitEvent(line 262), andV2Update(line 274) consistently prevent panics when accessing nested V2 fields.
85-96: Verify nil handling for Source parameter.The function accepts pointer
*shared.V2StageSendSourcebut doesn't check ifsrcis nil before accessingsrc.Wallet,src.Account, andsrc.Paymenton lines 87-92. Although the call site guards the parent struct (line 143), it doesn't guarantee theSourcefield itself is non-nil. In Go, a pointer field can be nil even when the parent is not.Additionally, the similar
stageDestinationNamefunction (lines 98-108) exhibits the same pattern. If either can receive nil, add nil checks:func stageSourceName(src *shared.V2StageSendSource) string { if src == nil { return "unknown_source_type" } switch { // ... rest of logicVerify this with the formance-sdk-go documentation to confirm whether the Source field can be nil or if nil is a contractual impossibility.
cmd/orchestration/instances/send_event.go (1)
52-57: Code syntax and structure appear correct; verify V2 API contract with SDK documentation.The migration from V1 to V2 is properly implemented with correct field mapping (args[0] → InstanceID, args[1] → Name) and appropriate error handling. However, without access to the formance-sdk-go SDK's struct definitions, the V2 API field contract cannot be conclusively verified. Confirm that V2SendEventRequest and V2SendEventRequestBody in the SDK have compatible field names and types.
cmd/orchestration/instances/list.go (1)
67-88: V2ListInstanceswiring and instance mapping look goodThe move to
Orchestration.V2.ListInstanceswithoperations.V2ListInstancesRequestand the mapping from[]shared.V2WorkflowInstanceto the table-friendlyWorkflowInstance(including RFC3339 timestamps and a nil-safeTerminatedAt) is coherent and aligns with the rest of the V2 migration.cmd/orchestration/instances/show.go (2)
18-19: Storing raw V2 instance plus converted workflow keeps concerns separatedHaving
InstancesShowStorehold the wire type (shared.V2WorkflowInstance) alongside a convertedshared.Workflowgives you flexibility: the table uses V2 fields directly while the detailed printer continues to work with the stable shared types. No changes needed here.
101-119: V2WorkflowInstance → WorkflowInstance/StageStatus conversion looks completeThe conversion from
shared.V2WorkflowInstanceintoshared.WorkflowInstance, including theStatusslice viafctl.Mapon[]shared.V2StageStatus, appears to preserve all the key fields (ID,WorkflowID, timestamps,Terminated,Error, and per‑stage timestamps/errors). This keepsinternal.PrintWorkflowInstanceoperating on the stable shared types while still sourcing data from V2.cmd/orchestration/workflows/delete.go (1)
47-51: DeleteWorkflow migration to V2 is straightforward and consistentThe switch to
Orchestration.V2.DeleteWorkflowwithoperations.V2DeleteWorkflowRequest{FlowID: args[0]}is a clean V2 migration and keeps the command’s behavior intact.cmd/orchestration/workflows/run.go (2)
72-77: V2RunWorkflowrequest wiring preserves existing semanticsThe move to
Orchestration.V2.RunWorkflowwithoperations.V2RunWorkflowRequestcorrectly passes variables inRequestBody, thewaitflag by pointer, and theWorkflowIDfromargs[0], so the behavior of the run command (including optional waiting) is preserved.
84-93: V2WorkflowInstance → WorkflowInstance mapping is aligned with other commandsThe mapping from
response.V2RunWorkflowResponse.Dataintoshared.WorkflowInstance(IDs, timestamps,Terminated,TerminatedAt,Error) matches the pattern used in the instances show path, so downstream consumers likePrintWorkflowInstancecontinue to work on the shared type without needing to know about V2.
gfyrag
force-pushed
the
chore/clean-v1-support
branch
from
1ad642c to
f1182d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
cmd/orchestration/triggers/occurrences/list.go (2)
78-83: Fix inverted logic causing nil pointer panic.The logic is inverted: it returns an empty string when
WorkflowInstanceIDis NOT nil, and attempts to dereference it when it IS nil. This will panic at runtime.Apply this diff to fix the logic:
func() string { - if src.WorkflowInstanceID != nil { + if src.WorkflowInstanceID == nil { return "" } return *src.WorkflowInstanceID }(),
85-86: Add nil checks before accessing WorkflowInstance fields.Lines 85-86 access
src.WorkflowInstance.Terminatedandsrc.WorkflowInstance.TerminatedAtwithout checking ifWorkflowInstanceis nil. This can cause a panic, as evidenced by the nil check at line 91 which showsWorkflowInstancecan be nil.Apply this diff to add nil safety:
-fctl.BoolToString(src.WorkflowInstance.Terminated), -src.WorkflowInstance.TerminatedAt.Format(time.RFC3339), +func() string { + if src.WorkflowInstance == nil { + return "" + } + return fctl.BoolToString(src.WorkflowInstance.Terminated) +}(), +func() string { + if src.WorkflowInstance == nil || src.WorkflowInstance.TerminatedAt == nil { + return "" + } + return src.WorkflowInstance.TerminatedAt.Format(time.RFC3339) +}(),cmd/orchestration/triggers/list.go (1)
95-95: Add nil check for Name field before dereferencing, consistent with Filter pattern.The code at line 95 dereferences
src.Namewithout a nil check. Lines 100-104 demonstrate the correct pattern already used forFilter. Apply the same nil-check protection:func() string { if src.Name == nil { return "" } return *src.Name }()This ensures safety if the
Namefield can be nil.
♻️ Duplicate comments (1)
cmd/orchestration/instances/show.go (1)
85-89: GuardTerminatedAtbefore formattingTerminated instances can arrive without a
TerminatedAtvalue; callingFormaton a nil pointer will panic and crash the CLI. Re-introduce the nil guard when printing the timestamp.- if c.store.WorkflowInstance.Terminated { - tableData = append(tableData, []string{pterm.LightCyan("Terminated at"), c.store.WorkflowInstance.TerminatedAt.Format(time.RFC3339)}) + if c.store.WorkflowInstance.Terminated && c.store.WorkflowInstance.TerminatedAt != nil { + tableData = append(tableData, []string{pterm.LightCyan("Terminated at"), c.store.WorkflowInstance.TerminatedAt.Format(time.RFC3339)}) }
🧹 Nitpick comments (1)
cmd/orchestration/workflows/run.go (1)
101-116: Extract shared V2Workflow-to-Workflow conversion helper to reduce duplication across 3+ files.Verification confirms the original concerns:
Code duplication verified: The V2Workflow to shared.Workflow conversion logic appears in at least 3 locations:
cmd/orchestration/workflows/show.go:67cmd/orchestration/workflows/run.go:114cmd/orchestration/instances/show.go:74Type conversion is safe: V2Workflow.Config is a JSON object (map[string]interface{}) and shared.WorkflowConfig handles JSON data similarly, making the direct type conversion structurally sound.
No existing helper: The codebase lacks a converter function, making this duplication a clear refactoring opportunity.
Extracting a shared helper function (e.g., in
cmd/orchestration/internal/converters.go) would eliminate duplication across multiple files and improve maintainability. The suggested helper function in the original comment is appropriate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (21)
cmd/orchestration/instances/describe.go(9 hunks)cmd/orchestration/instances/list.go(1 hunks)cmd/orchestration/instances/send_event.go(1 hunks)cmd/orchestration/instances/show.go(3 hunks)cmd/orchestration/instances/stop.go(1 hunks)cmd/orchestration/triggers/create.go(2 hunks)cmd/orchestration/triggers/delete.go(1 hunks)cmd/orchestration/triggers/list.go(1 hunks)cmd/orchestration/triggers/occurrences/list.go(3 hunks)cmd/orchestration/triggers/show.go(1 hunks)cmd/orchestration/workflows/create.go(1 hunks)cmd/orchestration/workflows/delete.go(1 hunks)cmd/orchestration/workflows/list.go(2 hunks)cmd/orchestration/workflows/run.go(2 hunks)cmd/orchestration/workflows/show.go(1 hunks)cmd/root.go(0 hunks)cmd/search/root.go(0 hunks)cmd/search/views/accounts.go(0 hunks)cmd/search/views/assets.go(0 hunks)cmd/search/views/payment.go(0 hunks)cmd/search/views/transactions.go(0 hunks)
💤 Files with no reviewable changes (6)
- cmd/search/views/payment.go
- cmd/search/root.go
- cmd/search/views/accounts.go
- cmd/root.go
- cmd/search/views/assets.go
- cmd/search/views/transactions.go
🚧 Files skipped from review as they are similar to previous changes (5)
- cmd/orchestration/triggers/delete.go
- cmd/orchestration/workflows/delete.go
- cmd/orchestration/triggers/create.go
- cmd/orchestration/triggers/show.go
- cmd/orchestration/instances/send_event.go
🧰 Additional context used
🧬 Code graph analysis (6)
cmd/orchestration/workflows/list.go (1)
pkg/utils.go (1)
Map(22-28)
cmd/orchestration/instances/list.go (2)
pkg/flags.go (3)
Ptr(89-91)GetBool(26-33)GetString(35-41)pkg/utils.go (1)
Map(22-28)
cmd/orchestration/workflows/run.go (3)
cmd/orchestration/instances/list.go (1)
WorkflowInstance(16-22)pkg/stores.go (1)
GetStackStore(92-94)cmd/orchestration/workflows/list.go (1)
Workflow(16-21)
cmd/orchestration/instances/show.go (1)
pkg/utils.go (1)
Map(22-28)
cmd/orchestration/workflows/show.go (1)
cmd/orchestration/workflows/list.go (1)
Workflow(16-21)
cmd/orchestration/triggers/list.go (1)
pkg/utils.go (1)
Map(22-28)
🔇 Additional comments (9)
cmd/orchestration/instances/stop.go (1)
48-50: V1 to V2 migration verified as correct.The V2.CancelEvent method and V2CancelEventRequest type are from the external SDK (operations package) and are properly integrated. This change is consistent with the widespread V2 migration across orchestration (workflows, instances, triggers) and ledger operations throughout the codebase. The API call pattern, parameter structure, and error handling are all correct.
cmd/orchestration/workflows/create.go (2)
69-69: LGTM: V2 response handling updated correctly.The migration to
V2CreateWorkflowResponse.Data.IDis consistent with the V2 API structure.
54-64: Verify struct field compatibility with the formance-sdk-go SDK.The code imports
shared.WorkflowConfigandshared.V2WorkflowConfigfrom an external SDK (formance-sdk-go v3). While the code copies onlyNameandStagesfields toV2WorkflowConfig, the struct definitions cannot be verified locally or via public documentation. However, the pattern of copying only specific fields between different struct types does present a risk: ifWorkflowConfigcontains additional fields, they will be silently dropped.Verify with the formance-sdk-go v3 SDK documentation or source code that both structs contain the same fields (or only
NameandStagesare relevant), and confirm this is the intended behavior.cmd/orchestration/workflows/list.go (1)
59-76: LGTM: V2 API migration with proper nil safety.The migration to V2 is clean and includes appropriate nil checks for
Config.Name. The mapping fromV2Workflowto the internalWorkflowtype correctly extracts all necessary fields.cmd/orchestration/workflows/show.go (2)
53-59: LGTM: V2 GetWorkflow API migration.The migration to
V2.GetWorkflowwithV2GetWorkflowRequestis correct and uses the appropriateFlowIDparameter.
61-68: Verify type conversion safety between V2 and internal WorkflowConfig types with SDK maintainers or documentation.The direct type conversion
shared.WorkflowConfig(v2Workflow.Config)at line 67 is used consistently across the codebase (also incmd/orchestration/instances/show.go:74andcmd/orchestration/workflows/run.go:114). Both types come from the external SDK (github.com/formancehq/formance-sdk-go/v3/pkg/models/shared), and the consistent usage pattern without error handling suggests this conversion is intentional.However, since both type definitions reside in the external SDK rather than the codebase, field-level compatibility cannot be verified directly. Confirm with your SDK maintainers that
V2WorkflowConfigandWorkflowConfigare structurally compatible and that this direct type conversion does not risk data loss.cmd/orchestration/workflows/run.go (1)
72-93: LGTM: V2 API migration with explicit field mapping.The migration to
V2.RunWorkflowis correct, and the explicit field-by-field mapping fromV2WorkflowInstancetoshared.WorkflowInstanceis clear and maintainable. All necessary fields are properly mapped.cmd/orchestration/triggers/list.go (1)
55-75: LGTM! V2 API migration looks correct.The migration from V1 to V2 API and the mapping logic from
V2Triggertoshared.Triggeris well-implemented. The field mapping is comprehensive and correctly converts all relevant fields.cmd/orchestration/triggers/occurrences/list.go (1)
52-59: LGTM! V2 API migration is correct.The migration to V2 API and the correct access path through
response.V2ListTriggersOccurrencesResponse.Cursor.Datalooks good.
| response, err := store.Client().Orchestration.V2.ListInstances(cmd.Context(), operations.V2ListInstancesRequest{ | ||
| Running: fctl.Ptr(fctl.GetBool(cmd, c.runningFlag)), | ||
| WorkflowID: fctl.Ptr(fctl.GetString(cmd, c.workflowFlag)), | ||
| }) |
There was a problem hiding this comment.
Remove implicit filters when flags are unset
The new request always sends running=false and workflowId="", so a plain instances list call now hides running instances and anything whose workflow ID isn’t empty. Only set these pointers when the user actually provided the flag/value.
- response, err := store.Client().Orchestration.V2.ListInstances(cmd.Context(), operations.V2ListInstancesRequest{
- Running: fctl.Ptr(fctl.GetBool(cmd, c.runningFlag)),
- WorkflowID: fctl.Ptr(fctl.GetString(cmd, c.workflowFlag)),
- })
+ req := operations.V2ListInstancesRequest{}
+ if cmd.Flags().Changed(c.runningFlag) {
+ req.Running = fctl.Ptr(fctl.GetBool(cmd, c.runningFlag))
+ }
+ if workflowID := fctl.GetString(cmd, c.workflowFlag); workflowID != "" {
+ req.WorkflowID = fctl.Ptr(workflowID)
+ }
+
+ response, err := store.Client().Orchestration.V2.ListInstances(cmd.Context(), req)🤖 Prompt for AI Agents
In cmd/orchestration/instances/list.go around lines 67 to 70, the request always
sends Running=false and WorkflowID="" because the pointer fields are set
unconditionally; change the request construction to only include Running and
WorkflowID when the corresponding flag was actually provided by the user (use
cmd.Flags().Changed(c.runningFlag) and cmd.Flags().Changed(c.workflowFlag) or
equivalent) so you only set the pointers when flags are present, leaving them
nil otherwise.
No description provided.