-
Notifications
You must be signed in to change notification settings - Fork 328
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
Extends 'teams app remove' command with support for name option. Closes #5445 #5454
Conversation
Thank you @nanddeepn, we'll try to review it ASAP! |
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.
Hi @nanddeepn, great work! I've got a few small comments before we merge this 👍
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.
Looks great @nanddeepn, I'll merge it and fix a few last things while we're at it. 👏
headers: { | ||
accept: 'application/json;odata.metadata=none' | ||
if (this.verbose) { | ||
await logger.logToStderr(`Removing app with ID ${args.options.id}`); |
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.
await logger.logToStderr(`Removing app with ID ${args.options.id}`); | |
await logger.logToStderr(`Removing app with ID ${appId}`); |
resultAsKeyValuePair[obj.id] = obj; | ||
}); | ||
|
||
return Cli.handleMultipleResultsFound(`Multiple Teams apps with name '${options.name}' found.`, resultAsKeyValuePair); |
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.
You should await this and type the result.
return Cli.handleMultipleResultsFound(`Multiple Teams apps with name '${options.name}' found.`, resultAsKeyValuePair); | |
const result = await Cli.handleMultipleResultsFound<{ id: string; }>(`Multiple Teams apps with name '${options.name}' found.`, resultAsKeyValuePair); | |
return result.id; |
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.
oh, and we also missed a test case for this :-)
if (options.id) { | ||
return options.id; | ||
} | ||
|
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.
We should add a verbose logging line here
Merged manually, thank you for the awesome work! 👏🎉 |
Extends
teams app remove
command with support for specifying thename
option. Closes #5445