-
Notifications
You must be signed in to change notification settings - Fork 20
fix: add mdapi:deploy:cancel command, refactor base classes to support MDAPI and SOURCE stash keys #301
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
…t MDAPI and SOURCE stash keys
mshanemc
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.
approved with non-blocking suggestions/questions
| protected deployResult: DeployResult; | ||
| /** | ||
| * Request a report of an in-progess or completed deployment. | ||
| * Request a report of an in-progress or completed deployment. |
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.
+1
|
|
||
| export abstract class SourceCommand extends SfdxCommand { | ||
| public static readonly DEFAULT_SRC_WAIT_MINUTES = 33; | ||
| public static readonly DEFAULT_WAIT_MINUTES = 33; |
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 the idea that the mdapi:deploy command will just have it's own flag that's set to zero (that is, just not using the default)?
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.
hmmm good point. for mdapi:deploy:cancel in toolbelt it was using the source:deploy default, it seemed weird to have this variable mention SRC when it's now used in both source and mdapi
very open to reverting this back
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 it's good since 33 seems to be most everywhere. Just making sure we weren't gonna come back and put a DEFAULT_MDAPI_WAIT_MINUTES or whatever.
| describe('mdapi:deploy:cancel', () => { | ||
| it('will cancel an mdapi deploy via the stash.json', () => { | ||
| execCmd('force:source:convert --outputdir mdapi'); | ||
| // TODO: once mdapi:deploy is migrated switch to execCmd |
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.
+1 for the TODO
|
QA notes: 👎🏻 run --help ✅ run without a username (good error) vs toolbelt 👎🏻 ask to cancel a jobId that doesn't exist. Toolbelt is similarly bad. ✅ cancel a job that already succeeded fails with 👎🏻 cancel a jobid that is a CustomObject's id fails with 👎🏻 cancel a job, then cancel it again fails with
that's changed to ✅ delete |
What does this PR do?
moves
mdapi:deploy:cancelcommand and messagesextends
plugin-sourcebase classes correctlyupdates base classes to support
MDAPI_DEPLOYstash keyadds UTs
adds NUTs
source:deploy:cancel,source:deploy:report,mdapi:deploy:cancelall validate the passed in deploy ID parameterfixes issue when stash is missing a requested key/value
What issues does this PR fix or reference?
@W-9603715@