Skip to content
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

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

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Conversation

cristiand391
Copy link
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
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.

@@ -37,34 +38,45 @@ type ResumeCommandIDs = 'data import resume' | 'data update resume';
*
* 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
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.

src/bulkIngest.ts Outdated Show resolved Hide resolved

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
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
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
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).

src/commands/data/export/resume.ts Outdated Show resolved Hide resolved
@@ -43,7 +43,7 @@ export abstract class ResumeBulkCommand extends SfCommand<BulkResultV2> {
summary: messages.getMessage('flags.wait.summary'),
unit: 'minutes',
min: 0,
defaultValue: 0,
defaultValue: 5,
Copy link
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
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
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
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
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
Member Author

Choose a reason for hiding this comment

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

@@ -43,7 +43,7 @@ export abstract class ResumeBulkCommand extends SfCommand<BulkResultV2> {
summary: messages.getMessage('flags.wait.summary'),
unit: 'minutes',
min: 0,
defaultValue: 0,
defaultValue: 5,
Copy link
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
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).

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.

2 participants