-
Notifications
You must be signed in to change notification settings - Fork 14
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
W-17037056 feat: add data update bulk/resume
commands
#1098
Conversation
50d03b6
to
172c7d5
Compare
[skip ci]
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.
just a few thoughts/suggestions - otherwise looks good 👍
src/bulkIngest.ts
Outdated
const timeout = opts.async ? Duration.minutes(0) : opts.wait ?? Duration.minutes(0); | ||
const async = timeout.milliseconds === 0; | ||
|
||
const baseUrl = opts.conn.getAuthInfoFields().instanceUrl as string; |
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.
should we handle the possibility that .instanceUrl
doesn't exist or undefined instead of using as string
?
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 can't find the slack thread but IIRC we said stuff like instanceUrl
should always be in auth files but changing the types ins sfdx-core now would be a big change, I'll wrap it in ensureString
👍🏼 .
const result = execCmd<DataImportBulkResult>( | ||
`data import bulk --file ${csvFile} --sobject Account --wait 10 --column-delimiter PIPE --json`, | ||
{ ensureExitCode: 0 } | ||
).jsonOutput?.result as DataImportBulkResult; |
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.
is as DataImportBulkResult
just there to remove the | undefined
? If so, why not use optional chaining instead of using an assertion?
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 started doing the assertion in other NUTs and just carried the assertion, I can remove it.
QA 🟢
🟡 It fails as expected but I'm wondering if there's a way to auto-detect the delimiter??
🟢
🟡 As I was trying to figure out how to use the new command, I encountered this:
The Looks like I needed to have an 🟢
🟢 handles aborted job
|
… into cd/bulk-update
data update bulk/resume
commands data update bulk/resume
commands
What does this PR do?
data update bulk/resume
commands--column-delimiter
flag todata import bulk
data import bulk
logic into a genericbulkIngest
func that will be used by all bulkIngest-related commands (data import/update bulk
in this PR,data delete/upsert bulk
will be migrated in a separate PR)What issues does this PR fix or reference?
@W-17037056@