Orchestrate the CRR Apply Actions chain with useChainedMutations - #1248
Conversation
Hello hervedombya,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Well-structured PR — the chain orchestration, the two-source state merge in |
0669dbd to
adbe78f
Compare
Review by Claude Code |
adbe78f to
0bda58b
Compare
| {setup.isLoading && ( | ||
| <Button type="button" variant="outline" label="Cancel" onClick={setup.cancel} /> | ||
| )} | ||
| <Button type="button" variant="outline" label="Exit" disabled={setup.isLoading} onClick={() => prev(props)} /> |
There was a problem hiding this comment.
disabled={setup.isLoading} only guards during the configurator stream step. During wizard-run chain links (import certificate, create account, create location, etc.), Exit remains enabled — the user can navigate away mid-chain. The in-flight mutation completes in the background but the chain stops, leaving the system partially provisioned. Re-entering the step restarts the chain from scratch, which can fail on non-idempotent steps (e.g. create-source-account → "entity already exists").
The same gap applies after the configurator stream finishes but before create-location / create-replication-rule complete — setup.isLoading is false again so Exit re-enables.
Consider deriving an isChainRunning flag from the chain steps and using it instead of setup.isLoading.
— Claude Code
| }; | ||
|
|
||
| return useMutation({ | ||
| mutationFn: async (location: LocationV1) => { |
There was a problem hiding this comment.
The polling loop has no cancellation mechanism. If the component unmounts mid-poll (user clicks Exit or Cancel during this step), all 120 iterations continue running in the background. Consider threading an AbortSignal through and checking signal.aborted inside the loop to break early on unmount.
— Claude Code
|
|
||
| return useMutation({ | ||
| mutationFn: async ({ certificate }: { certificate: string }) => { | ||
| if (isCertificateAlreadyImported(zenkoCR, certificate)) { |
There was a problem hiding this comment.
zenkoCR is captured at render time, not at mutation execution time. If a background refetch updates the Zenko CR between render and the chain reaching this step, the idempotency check uses stale data and a duplicate certificate entry could be appended. The window is small in practice (chain fires quickly after mount), but for full correctness the fresh state could be read inside the mutation via queryClient.fetchQuery(getZenkoCRQuery()).
— Claude Code
|
0bda58b to
3552317
Compare
| configs.push({ id: 'create-location', label: 'Create Location', mutation: createLocation }); | ||
| resolvers['create-location'] = (prev) => { | ||
| const result = configuratorResult(prev); | ||
| return result ? buildCRRLocation(locationName, result) : undefined; |
There was a problem hiding this comment.
The create-location and create-replication-rule resolvers silently return undefined when configuratorResult(prev) is missing. Since the chain runs sequentially and should stop on failure, this path should be unreachable — but if it is reached (e.g. a bug in useChainedMutations), the downstream mutation receives undefined and fails with a confusing runtime error rather than a descriptive one.
Consider throwing explicitly so the failure is diagnosable. Same applies to the create-replication-rule resolver at line 190.
— Claude Code
| <Button type="button" variant="outline" label="Cancel" onClick={setup.cancel} /> | ||
| )} | ||
| <Button type="button" variant="outline" label="Exit" disabled={isRunning} onClick={() => prev(props)} /> | ||
| {setup.isLoading && <Button type="button" variant="outline" label="Cancel" onClick={setup.cancel} />} |
There was a problem hiding this comment.
During non-configurator long-running steps (e.g. create-location polling up to 60s), isRunning is true so Exit is disabled, but Cancel only renders when setup.isLoading. The user has no way to interrupt a 60-second reconciliation wait. If this is intentional (to prevent partial provisioning on exit), worth a brief comment; otherwise consider extending Cancel to cover the full chain run.
— Claude Code
|
LGTM — well-structured chain orchestration with clean state merging and good unit test coverage on the pure-logic layers ( |
3552317 to
8ba6f3e
Compare
| 'create-source-bucket': ['create-source-bucket', 'create-source-bucket-versioning'], | ||
| 'create-location': ['create-location'], | ||
| 'create-replication-rule': ['create-replication-rule'], | ||
| }; |
There was a problem hiding this comment.
If assume-source-role fails (STS unreachable, wrong ARN, propagation delay), the chain stops but no row shows an error — assume-source-role has no entry in WIZARD_ROW_LINKS, so the failure is invisible. The user sees the remaining rows stuck on "Pending..." with no indication of what went wrong and no Retry button.
Adding it to the create-source-bucket entry would surface the error and let Retry re-run the failed link:
| }; | |
| const WIZARD_ROW_LINKS: Partial<Record<StepId, string[]>> = { | |
| 'import-destination-certificate': ['import-destination-certificate'], | |
| 'create-source-account': ['create-source-account'], | |
| 'create-source-bucket': ['assume-source-role', 'create-source-bucket', 'create-source-bucket-versioning'], | |
| 'create-location': ['create-location'], | |
| 'create-replication-rule': ['create-replication-rule'], | |
| }; |
— Claude Code
Review by Claude Code |
8ba6f3e to
a965620
Compare
| return useMutation({ | ||
| mutationFn: async (location: LocationV1) => { | ||
| const referenceVersion = await runningConfigurationVersion(); | ||
| await createLocation.mutateAsync(location); |
There was a problem hiding this comment.
Retry after a polling timeout will call createLocation.mutateAsync(location) again, attempting to re-create a location that already exists. Unlike the certificate import (which checks isCertificateAlreadyImported first), this mutation has no idempotency guard.
If the overlay is slow to reconcile and the 60s timeout fires, the location was already created successfully — the user just needs to re-poll, not re-create. On retry the management API will likely reject the duplicate, surfacing a confusing "already exists" error instead of resuming the wait.
Consider either catching the "already exists" response and continuing to poll, or splitting the logic so retry only re-polls when the location already exists.
— Claude Code
Review by Claude Code |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## development/4 #1248 +/- ##
=================================================
+ Coverage 62.85% 63.79% +0.94%
=================================================
Files 234 256 +22
Lines 8948 9454 +506
Branches 2435 2559 +124
=================================================
+ Hits 5624 6031 +407
- Misses 3314 3412 +98
- Partials 10 11 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a965620 to
2605298
Compare
|
2605298 to
454b460
Compare
|
LGTM |
|
/approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue ARTESCA-16787. Goodbye hervedombya. The following options are set: approve |
3 participants
TL;DR
The CRR wizard's Apply Actions step now actually runs the full provisioning sequence — it drives the wizard-side steps and the crr-configurator destination stream as one chained sequence, instead of only rendering the backend stream.
Context
Part of the ARTESCA CRR Provisioning Wizard (ARTESCA-16787). The step contract (12 canonical rows) landed earlier; this PR makes the step orchestrate them for real: some steps run in the browser (certificate import, source account/bucket, location, replication rule), the middle ones run on the destination via the crr-configurator NDJSON stream.
Approach
useChainedMutations(the ISV pattern) runs the sequence. The chain has more links than visible rows —buildStepViewsmerges two state sources into the 12 contract rows:import-destination-certificate(idempotent)create-source-account(when new)assume-source-rolecreate-source-bucket+create-source-bucket-versioningconfigurator-setup(one link)create-location(waits for overlay reconcile)create-replication-ruleTwo things worth knowing: the destination
crr-roleARN from the stream'sSetupResultis what makes the source replication rule'sRolethe composites3-replication-role,<roleArn>pair the CRR procedure requires; andcreate-locationonly resolves once the overlay has reconciled, because the replication rule references the location by name.Screenshots
Route
/create-crr-configuration.Apply Actions — the 12-row provisioning table, all pending before the run (Success / Failed + inline Retry appear per row as the chain progresses):
Configure — the step that feeds it (source account, destination connection + certificate, destination account, replication rule):
Review focus
ApplyActionsStep.tsx › useChainedMutations config + variable resolvers— the chain assembly and the data-flow from the streamSetupResultintocreate-location/create-replication-rule; a wrong resolver mis-provisions replication.steps.ts › buildStepViews/retryLinkIdForRow— merges configurator NDJSON events + wizard chain-link statuses into the 12 rows; per-row Retry re-runs the failed link of a folded row.useCreateCRRLocationMutation.ts— polls the running-config version so the location is reconciled before the replication rule runs (60s timeout, cancelled on unmount).How to test
References
__mocks__/@scality/data-browser-librarygainsuseSetBucketVersioning/useSetBucketReplication(additive; existing ISV tests unaffected).