-
Notifications
You must be signed in to change notification settings - Fork 220
[Bulk Operations CLI] show live progress updates when --watch is provided
#6595
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
--watch flag--watch is provided
Coverage report
Show new covered files 🐣
Show files with reduced coverage 🔻
Test suite run success3402 tests passing in 1384 suites. Report generated by 🧪jest coverage report action from 4c19852 |
|
We detected some changes at Caution DO NOT create changesets for features which you do not wish to be included in the public changelog of the next CLI release. |
3d9e66e to
5d6b5d5
Compare
5d6b5d5 to
153d57c
Compare
153d57c to
c324f49
Compare
2e960b1 to
594e201
Compare
446c3cf to
2b6b260
Compare
594e201 to
500737d
Compare
68dff49 to
c4027ba
Compare
c4027ba to
db8e837
Compare
569eae4 to
c9b42e0
Compare
| } else { | ||
| renderSuccess({headline, customSections}) | ||
| } | ||
| break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nit] Would it be better to use renderWarning here for other cases? Not too sure so would like to hear your opinion.
Like
| break | |
| break | |
| case 'CANCELED': | |
| case 'CANCELING': | |
| case 'EXPIRED': | |
| renderWarning({headline, customSections}) | |
| break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I considered this, no strong feelings either way. Happy to change it for now if you like. Let's check in with @nickwesselman about this once he's back from Thanksgiving.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, those states should be shown as errors because the query didn't finish successfully, so I think it's ok as it is.
packages/app/src/cli/services/bulk-operations/watch-bulk-operation.ts
Outdated
Show resolved
Hide resolved
c9b42e0 to
5270287
Compare
|
Everything looks good to me! Great tophatting video btw 👍 |
5270287 to
d3ffc48
Compare
d3ffc48 to
0804636
Compare
|
/snapit |
|
🫰✨ Thanks @gonzaloriestra! Your snapshot has been published to npm. Test the snapshot by installing your package globally: npm i -g --@shopify:registry=https://registry.npmjs.org @shopify/[email protected]Caution After installing, validate the version by running just |
gonzaloriestra
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Tested that it works as expected.
I left a bunch of suggestions, mostly about UX.
| const createdOperation = bulkOperationResponse?.bulkOperation | ||
| if (createdOperation) { | ||
| if (watch) { | ||
| const finishedOperation = await watchBulkOperation(adminSession, createdOperation.id) | ||
| renderBulkOperationResult(finishedOperation) | ||
| } else { | ||
| renderBulkOperationResult(createdOperation) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about simplifying this way? Improving the error message with something more useful if possible.
| const createdOperation = bulkOperationResponse?.bulkOperation | |
| if (createdOperation) { | |
| if (watch) { | |
| const finishedOperation = await watchBulkOperation(adminSession, createdOperation.id) | |
| renderBulkOperationResult(finishedOperation) | |
| } else { | |
| renderBulkOperationResult(createdOperation) | |
| } | |
| } | |
| let operation = bulkOperationResponse?.bulkOperation | |
| if (!operation) throw new AbortError('Something went wrong when creating the bulk operation') | |
| if (watch) operation = await watchBulkOperation(adminSession, createdOperation.id) | |
| renderBulkOperationResult(operation) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Fair suggestion, thanks for thinking it through!
I think I like the way it is currently more, even though it's more lines. The current structure has different names for the operation -- finishedOperation vs createdOperation -- which communicates explicitly the expected state of the operation in each case. That makes it easier to grok at a glance for me, but I dunno, maybe that's just a me thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm opting to leave it as is for now, but if you feel strongly let me know and I'm happy to adjust in a follow-up :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, I don't have a strong opinion.
What I don't like about the current approach is that it does nothing when createdOperation is undefined. But maybe that's not possible...
| } else { | ||
| renderSuccess({headline, customSections}) | ||
| } | ||
| break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO, those states should be shown as errors because the query didn't finish successfully, so I think it's ok as it is.
packages/app/src/cli/services/bulk-operations/format-bulk-operation-status.ts
Show resolved
Hide resolved
packages/app/src/cli/services/bulk-operations/format-bulk-operation-status.ts
Outdated
Show resolved
Hide resolved
packages/app/src/cli/services/bulk-operations/execute-bulk-operation.ts
Outdated
Show resolved
Hide resolved
…d print live progress updates
0804636 to
c8e62d6
Compare
c8e62d6 to
4c19852
Compare

Issue: https://github.com/shop/issues-api-foundations/issues/1093
WHY are these changes introduced?
We want users to be able to pass a
--watchflag, so they can monitor the progress and completion of their bulk operations directly from the CLI.WHAT is this pull request doing?
GetBulkOperationByIdto fetch the status and details of a bulk operationwatchBulkOperationthat polls a bulk operation and displays live progress updates, using the newrenderSingleTaskasync functionality added by Refactor renderSingleTask to allow an async process to update current status #6631--watchflag to theapp executecommand that controls whether to usewatchBulkOperationHow to test your changes?
Bulk queries
Verify that when
--watchis provided, we see live updates printed to the terminal, then eventually a final screen when it finishes:export-with-watch.mov
Next, verify that when
--watchis not provided, theexecutecommand prints some info about the bulk operation and returns right away, just like before this PR:export-without-watch.mov
Then do the same tests for bulk mutations:
Bulk mutations
With
--watch:import-with-watch.mov
Without
--watch:import-without-watch.mov