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

Extends 'teams user app remove' command with support for name option. Closes #5447 #5460

Closed
wants to merge 11 commits into from

Conversation

nanddeepn
Copy link
Contributor

Extends 'teams user app remove' command with support for specifying the name option. Closes #5447

@Jwaegebaert
Copy link
Contributor

Thanks for your effort @nanddeepn. We'll review it shortly

@waldekmastykarz waldekmastykarz self-assigned this Oct 8, 2023
Copy link
Member

@waldekmastykarz waldekmastykarz left a comment

Choose a reason for hiding this comment

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

When running tests after rebasing onto latest main, I'm getting a prompt for userId (using default settings). Seems like something is not mocked somewhere. Could you please fix it before we merge the PR?

image

@waldekmastykarz waldekmastykarz marked this pull request as draft October 8, 2023 12:38
@nanddeepn nanddeepn marked this pull request as ready for review October 8, 2023 15:14
@waldekmastykarz waldekmastykarz added hacktoberfest-accepted Accept for hacktoberfest, will merge later and removed hacktoberfest-accepted Accept for hacktoberfest, will merge later labels Oct 27, 2023
Copy link
Member

@waldekmastykarz waldekmastykarz left a comment

Choose a reason for hiding this comment

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

Hey @nanddeepn, it seems that you and Reshmee have both submitted a PR to the same file. We're about to merge her PR. Would you be willing to resolve conflicts on yours so that we can pull it in too? I appreciate your help 😊

@waldekmastykarz waldekmastykarz marked this pull request as draft November 9, 2023 18:45
@nanddeepn nanddeepn marked this pull request as ready for review November 10, 2023 06:46
Copy link
Member

@waldekmastykarz waldekmastykarz left a comment

Choose a reason for hiding this comment

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

Hey @nanddeepn, I'm still getting conflicts when trying to rebase it onto latest main. Could you please rebase with the last main and squash your old commits so that we can clearly merge it without losing any functionality? Thank you!

@waldekmastykarz waldekmastykarz marked this pull request as draft November 11, 2023 13:35
}

if (response.value.length > 1) {
throw `Multiple Teams apps with name ${args.options.name} found. Please choose one of these ids: ${response.value.map(x => x.id).join(', ')}`;
Copy link
Member

@waldekmastykarz waldekmastykarz Nov 11, 2023

Choose a reason for hiding this comment

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

Rather than throwing an error here, we should use Cli.handleMultipleResultsFound() which shows a disambiguation prompt or throws error depending on a user's settings. Sorry I missed it earlier.

@nanddeepn
Copy link
Contributor Author

Hi @waldekmastykarz
Raised new PR at #5662. Closing this.

@nanddeepn nanddeepn closed this Nov 13, 2023
@nanddeepn nanddeepn deleted the issue-5447 branch January 2, 2024 04:37
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.

[Enhancement]: Extend teams user app remove command with --name option
3 participants