Skip to content

W-16971607 feat: multi-stage-output/new csv flags#1110

Merged
cristiand391 merged 28 commits intomainfrom
cd/mso-new-flags
Nov 26, 2024
Merged

W-16971607 feat: multi-stage-output/new csv flags#1110
cristiand391 merged 28 commits intomainfrom
cd/mso-new-flags

Conversation

@cristiand391
Copy link
Copy Markdown
Member

@cristiand391 cristiand391 commented Oct 31, 2024

What does this PR do?

  • Adds --line-ending flag to bulk commands that take a CSV file as input
  • update sf data delete/upsert bulk commands to use MSO

What issues does this PR fix or reference?

@W-16971607@
forcedotcom/cli#3061

@cristiand391 cristiand391 marked this pull request as ready for review November 8, 2024 15:14
@cristiand391 cristiand391 requested a review from a team as a code owner November 8, 2024 15:14
options: {
connection: (await Org.create({ aliasOrUsername: entry.username })).getConnection(),
},
};
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simplified resolveResumeOptionsFromCache so that it matches data import/export's cache resolver.
The previous implementation had a few unused properties and one (undocumented?) functionality:
passing a job ID that didn't exist in the cache wouldn't cause an error, the resolver would return it as if it was found in the local cache.
Added code ƒor specific bulk commands using this to avoid breaking changes.

*
* It will create the specified bulk ingest job, set up the oclif/MSO stages and return the job info.
* */
// eslint-disable-next-line complexity
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had to add this after adding the warning/deprecation/validation checks.


if (opts.verbose && !['delete', 'hardDelete', 'upsert'].includes(opts.operation)) {
throw new SfError(
'Verbose mode is limited for `sf data delete/upsert bulk` for backwards-compat only and will be removed after March 2025.'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"backwards-compat" - @jshackell-sfdc

jobIdOrMostRecent: string | boolean;
jsonEnabled: boolean;
wait: Duration;
warnFn: (message: SfCommand.Warning) => void;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

were you matching a pattern here? passing in a logFn/warnFn?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep:

logFn: (message: string) => void;

to allow using command's this.log/warning methods here (need to wrap in arrow function for this scoping).

unit: 'minutes',
min: 0,
defaultValue: 0,
defaultValue: 5,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will affect commands that don't currently pass --wait and that are expecting async behavior

Copy link
Copy Markdown
Member Author

@cristiand391 cristiand391 Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but given the current behavior I think it's a better (and non-breaking) experience.

This base class is only used by sf data upsert/delete resume, without a default wait time these resume commands would return exit code 0 and suggest to re-run them again even if the job didn't finish:

Screenshot 2024-11-14 at 9 08 45 AM

data update/import resume also have --wait=5 as default.

// validation
if (opts.externalId && opts.operation !== 'upsert') {
throw new SfError('External ID is only required for `sf data upsert bulk.');
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inlined error msg b/c it's for devs only to avoid passing externalId when it's not needed.

throw new SfError(
'Verbose mode is limited for `sf data delete/upsert bulk` for backwards-compat only and will be removed after March 2025.'
);
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto, dev-only error to block --verbose on new bulk commands.

columnDelimiter,
}).catch((err) => {
stages.stop('failed');
throw err;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chained catch to set failed stage properly (and avoid passing stages to createIngestJob)

'Resuming a synchronous operation via `sf data upsert/delete resume` will not be supported after March 2025.'
);
await opts.cache.createCacheEntryForRequest(job.id, ensureString(conn.getUsername()), conn.getApiVersion());
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unit: 'minutes',
min: 0,
defaultValue: 0,
defaultValue: 5,
Copy link
Copy Markdown
Member Author

@cristiand391 cristiand391 Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but given the current behavior I think it's a better (and non-breaking) experience.

This base class is only used by sf data upsert/delete resume, without a default wait time these resume commands would return exit code 0 and suggest to re-run them again even if the job didn't finish:

Screenshot 2024-11-14 at 9 08 45 AM

data update/import resume also have --wait=5 as default.

jobIdOrMostRecent: string | boolean;
jsonEnabled: boolean;
wait: Duration;
warnFn: (message: SfCommand.Warning) => void;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep:

logFn: (message: string) => void;

to allow using command's this.log/warning methods here (need to wrap in arrow function for this scoping).

@WillieRuemmele
Copy link
Copy Markdown
Contributor

QA Notes


on a call with Cristian to verify everything
✅ : flag deprecation warnings
✅ : resume an invalidated cache entry
✅ : json error information
✅ : auto-detect Column delims
✅ : using column delimiters
✅ : line endings

@cristiand391 cristiand391 merged commit 1d2cf30 into main Nov 26, 2024
@cristiand391 cristiand391 deleted the cd/mso-new-flags branch November 26, 2024 18:37
@amtrack
Copy link
Copy Markdown

amtrack commented Dec 16, 2024

@cristiand391 When importing a CSV file which has only 1 column it looks like the auto-detection of the column delimiter fails (and therefore also the import job):

Creating Account

Error (SfError): Failed to detect column delimiter used in data/Account.csv.

Job failed

I'm able to work around this by using --column-delimiter COMMA explicitly for the files having 1 column only.

Do you think there is a way to make this work without breaking anything?
If not, I think it's OK.

@cristiand391
Copy link
Copy Markdown
Member Author

@amtrack thanks for reporting, fixed it ⬆️ (available in the nightly channel)

@iowillhoit iowillhoit changed the title feat: multi-stage-output/new csv flags W-16971607 feat: multi-stage-output/new csv flags Jan 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants