-
Notifications
You must be signed in to change notification settings - Fork 333
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
add more options to spo user remove. closes #5522 #5806
Conversation
Thank you @reshmee011, we'll try to review it ASAP! |
@milanholemans , many thanks, not sure whether I have handled the additional options in the best possible way , it will good to have someone's else opinion. |
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.
@reshmee011 awesome work 👏
Thank you for this PR. You rock 🚀
I added some minor comments trying to suggest the solution or approach. Please give them a check before we proceed with the merge 👍
Co-authored-by: Adam Wójcik <[email protected]>
Co-authored-by: Adam Wójcik <[email protected]>
Co-authored-by: Adam Wójcik <[email protected]>
Co-authored-by: Adam Wójcik <[email protected]>
@Adam-it : Thanks for the awesome review, I learn a lot from the coding reviews. Hopefully I will finish reviewing all comments soon. |
No rush. Once again thank you for sticking with us 👍🤙. Let me know if you need any kind of help 🙂 |
@Adam-it : If I don't finish the review today, it will only over the weekend I might be able to work it as it is half term this week (school holidays for the girls) and my brother-in-law's wedding on Wednesday. |
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.
@reshmee011 Awesome work so far. Let's give it another round before we proceed 👍👏
You Rock 🤩😍
@reshmee011 I added a few more comments (mostly tiny details) 👍. Please double-check that GitHub tends to collapse comments so it's easy to overlook them and I suspect that is what happened also last time as I noticed 2 comments were not resolved (no problem with that👍) As always let me know if you need any help or if something is not clear 🙂 |
Co-authored-by: Adam Wójcik <[email protected]>
Co-authored-by: Adam Wójcik <[email protected]>
Co-authored-by: Adam Wójcik <[email protected]>
Co-authored-by: Adam Wójcik <[email protected]>
Co-authored-by: Adam Wójcik <[email protected]>
Co-authored-by: Adam Wójcik <[email protected]>
Co-authored-by: Adam Wójcik <[email protected]>
Co-authored-by: Adam Wójcik <[email protected]>
@reshmee011 I rechecked your question with the |
Co-authored-by: Adam Wójcik <[email protected]>
Co-authored-by: Adam Wójcik <[email protected]>
Co-authored-by: Adam Wójcik <[email protected]>
Co-authored-by: Adam Wójcik <[email protected]>
@Adam-it : I fell victim to the collapsed comment sections failing to resolve all. I think I have handled them all now. Sorry about this, I know it can frustrating to repeat the same thing. |
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.
Awesome work 👏👏👏
one small comment which I will sort out along the way 👍
@@ -14,38 +14,69 @@ m365 spo user remove [options] | |||
|
|||
```md definition-list | |||
`-u, --webUrl <webUrl>` | |||
: URL of the web to remove user | |||
: URL of the web to remove user. Use either `id` or `loginName` or `email`, `userName`, `entraGroupId`, or `entraGroupName`, but not all. |
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.
: URL of the web to remove user. Use either `id` or `loginName` or `email`, `userName`, `entraGroupId`, or `entraGroupName`, but not all. | |
: URL of the web to remove user. |
Ready to merge 🚀 |
Merged manually 👍 |
Closes #5522