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

All entra m365group commands should accept displayName option #6234

Closed

Conversation

MartinM85
Copy link
Contributor

Closes #6147

@milanholemans
Copy link
Contributor

Thank you @MartinM85 we'll try to review it ASAP

@milanholemans
Copy link
Contributor

Hi @MartinM85, could you resolve the merge conflicts by rebasing with the latest main, please?

@milanholemans milanholemans marked this pull request as draft August 24, 2024 17:47
@MartinM85
Copy link
Contributor Author

MartinM85 commented Aug 24, 2024

@milanholemans Two commands where extended with groupName option. What's the correct option name? Still groupDisplayName?

@milanholemans
Copy link
Contributor

We've renamed a bunch of commands from groupDisplayName to groupName #5616
So groupName is the option name we should use.

@MartinM85 MartinM85 force-pushed the feature/6147-m365group-add-displayName branch from fa44837 to 9335709 Compare August 24, 2024 20:15
@MartinM85 MartinM85 marked this pull request as ready for review August 24, 2024 20:28
@Adam-it Adam-it self-assigned this Oct 16, 2024
@Adam-it
Copy link
Member

Adam-it commented Oct 16, 2024

@MartinM85 may I kindly ask you to rebase the PR again before I may proceed with the review.
@milanholemans if we in this PR we are changing the groupDisplayName to groupName like in docs/docs/cmd/entra/m365group/m365group-conversation-post-list.mdx isn't this breaking change and this should be a pr-major as we may only do that now with the v10 release?

@Adam-it Adam-it marked this pull request as draft October 16, 2024 20:50
@milanholemans
Copy link
Contributor

@MartinM85 may I kindly ask you to rebase the PR again before I may proceed with the review. @milanholemans if we in this PR we are changing the groupDisplayName to groupName like in docs/docs/cmd/entra/m365group/m365group-conversation-post-list.mdx isn't this breaking change and this should be a pr-major as we may only do that now with the v10 release?

That wasn't really the purpose of the issue, but the commands should be aligned indeed. You are right.

@MartinM85 MartinM85 force-pushed the feature/6147-m365group-add-displayName branch from d002a7e to de1d242 Compare October 17, 2024 05:54
@MartinM85 MartinM85 marked this pull request as ready for review October 17, 2024 06:00
@MartinM85
Copy link
Contributor Author

@Adam-it Hope I've rebased everything correctly

@Adam-it Adam-it added the pr-major PR for the next major release label Oct 17, 2024
@Adam-it
Copy link
Member

Adam-it commented Oct 17, 2024

That wasn't really the purpose of the issue, but the commands should be aligned indeed. You are right.

Ok I will add pr-major label to this PR since now it is a breaking change

Copy link
Member

@Adam-it Adam-it left a comment

Choose a reason for hiding this comment

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

@MartinM85 Awesome work so far 👏👏👏👏
Before we proceed lets recheck the naming. TBH I do not quite feel that in some places/commands we refer to the same option as name and in other as displayName . I would try to align this
tagging @milanholemans to double check if maybe I missed anything from the initial issue

@Adam-it Adam-it marked this pull request as draft October 17, 2024 19:43
@MartinM85 MartinM85 marked this pull request as ready for review October 18, 2024 05:18
@MartinM85 MartinM85 requested a review from Adam-it October 18, 2024 05:18
@Adam-it Adam-it removed the pr-major PR for the next major release label Oct 21, 2024
Copy link
Member

@Adam-it Adam-it left a comment

Choose a reason for hiding this comment

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

@MartinM85 Awesome work 👏👏👏👏
I added just one small comment
and also when testing I noticed that the command m365 entra m365group teamify will not work if I use the newly added displayName option.

Would you like to give it a double check?

image

image

image

docs/docs/cmd/entra/m365group/m365group-remove.mdx Outdated Show resolved Hide resolved
@Adam-it Adam-it marked this pull request as draft November 5, 2024 21:51
@MartinM85
Copy link
Contributor Author

Hi @Adam-it, I had a typo in dispalyName.

@MartinM85 MartinM85 marked this pull request as ready for review November 6, 2024 07:26
@MartinM85 MartinM85 requested a review from Adam-it November 6, 2024 07:26
@Adam-it
Copy link
Member

Adam-it commented Nov 6, 2024

Hi @Adam-it, I had a typo in dispalyName.

Thanks for the quick turn around. I will try to make more reviews this week and for sure will retake this one as well

Copy link
Member

@Adam-it Adam-it left a comment

Choose a reason for hiding this comment

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

Perfect 🤩

@Adam-it
Copy link
Member

Adam-it commented Nov 14, 2024

Read to merge 🚀

@Adam-it
Copy link
Member

Adam-it commented Nov 15, 2024

Merged manually. Thank you for your awesome work! You Rock !🤩
@MartinM85 I noticed that you have been doing a looooot of awesome stuff in CLI repo 🚀. I was wondering if you would like to present some of your work, what you added to the CLI, or how you go about it, or why you do it, or how you use CLI.. etc to the community on one of the PnP Community calls?

@Adam-it Adam-it closed this Nov 15, 2024
@MartinM85
Copy link
Contributor Author

@Adam-it I can try to present some of my work on the PnP Community calls, but I can't say now what exactly. :)

@Adam-it
Copy link
Member

Adam-it commented Nov 22, 2024

@Adam-it I can try to present some of my work on the PnP Community calls, but I can't say now what exactly. :)

that's awesome to hear 🤩. You may more than one demo if you want 😉. There are plenty of awesome stuff you did that may be presented 👏
May I kindly ask you to fill out the form so that we may set a demo spot for you
https://forms.office.com/pages/responsepage.aspx?id=v4j5cvGGr0GRqy180BHbR8ke1rGfE-VNsUHrnMWCrL5UNjhIVEc4VERQUDNYOTM4MDdKN1ZLWFVBMSQlQCN0PWcu

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

All entra m365group commands should accept displayName option
3 participants