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

New command: aad group get #3786

Closed
martinlingstuyl opened this issue Oct 10, 2022 · 14 comments
Closed

New command: aad group get #3786

martinlingstuyl opened this issue Oct 10, 2022 · 14 comments
Assignees
Milestone

Comments

@martinlingstuyl
Copy link
Contributor

martinlingstuyl commented Oct 10, 2022

Usage

m365 aad group get [options]

Description

Retrieve information about a specific Azure AD Group

Options

Option Description
-i, --id [id] The object Id of the Azure AD group. Specify either id or title but not both.
-t, --title [title] The display name of the Azure AD group. Specify either id or title but not both.

Examples

Get the group information from Azure AD for the Group with the title Sales

m365 aad group get --title "Sales"

Additional Info

Because there's already a command that does this: aad o365group get we take the following steps:

  • copy the code of aad o365group get to aad group get and make it a brand new command
  • (in a new issue) update the code of aad o365group get so that it returns only O365 groups, rather than all groups as it's doing now? The current behavior might be confusing and lead to undesired effects if you think that you're working with an O365 group while in fact you're not.
@martinlingstuyl martinlingstuyl added new feature needs peer review Needs second pair of eyes to review the spec or PR labels Oct 10, 2022
@appieschot
Copy link
Member

We support deleted in our list command, do we need to include the same option when retrieving groups, or is there no difference when retrieving the details?

@arjunumenon
Copy link
Member

I think we already have similar command for getting O365 Group - aad o365group get.

@appieschot
Copy link
Member

@arjunumenon good point! We should in that case however refactor the o365group get as it will not return all groups since it contains a filter for unified groups only

https://github.com/pnp/cli-microsoft365/blob/main/src/m365/aad/commands/o365group/o365group-list.ts?plain=1#L87-L88

@arjunumenon
Copy link
Member

Good catch @appieschot 👍. When you are listing the groups, we are getting only unified groups. So when we enhance the command, we may have to update it.

But for this command, getting Group, when we are getting a particular group, we are not doing any filter as per here.
Getting group by ID
Util method for getting group.

@martinlingstuyl
Copy link
Contributor Author

I think we already have similar command for getting O365 Group - aad o365group get.

Aaahhh 😂 it's not a very logical place though, if you're looking for functionality to retrieve Azure AD groups.
I'd be a proponent for adding aad group get as an alias, or creating two commands with shared code.
How are your thoughts about that?

by the way, something similar goes up for the non-existent command aad group add. If I'd want to create an AAD security group.

@arjunumenon
Copy link
Member

arjunumenon commented Oct 11, 2022

How are your thoughts about that?

I would bank on the way to create a new command aad group get which would be an alias for aad o365group get.
Going forward, like you said we can eventually merge both so that there wouldn't be multiple command which does same thing.

by the way, something similar goes up for the non-existent command aad group add. If I'd want to create an AAD security group.

Like you said, for aad group add it would be make sense to have different command because currently aad o365group add only creates O365 group.

Here is my thought -
For aad group add, it will have options to select type of group (Currently they only support O365 group and Security Group).
If it is O365 group, it will internally call our aad o365group add command.

I will let other @pnp/cli-for-microsoft-365-maintainers comment on the approach

@appieschot
Copy link
Member

appieschot commented Oct 11, 2022

Whoops I confused list and get sorry ;-), so for aad o365group get we can use an alias :), and for add we would have new logic while remove could be an alias as well ;-)

@Adam-it
Copy link
Contributor

Adam-it commented Oct 12, 2022

@martinlingstuyl I agree. For me it's totally ok to have also this command on the aad level. Does not matter if we do it as alias or as some part of sharable code I guess users may actually be searching for this kind of commands in both namespaces so it's a very good idea to improve the 'visibility' of that.

Also since we have something very similar this will be a perfect case for a good first issue 💪

@waldekmastykarz
Copy link
Member

Wouldn't the ideal solution be to:

  • copy the code of aad o365group get to aad group get and make it a brand new command
  • update the code of aad o365group get so that it returns only O365 groups, rather than all groups as it's doing now? The current behavior might be confusing and lead to undesired effects if you think that you're working with an O365 group while in fact you're not.

@Adam-it
Copy link
Contributor

Adam-it commented Oct 15, 2022

even better 🙂. And will be consistent with the o365group list command which returns only M365 groups so it makes sense that the get command should return the same 👍

@waldekmastykarz
Copy link
Member

Hey @pnp/cli-for-microsoft-365-maintainers, do we have a consensus on the spec?

@Adam-it
Copy link
Contributor

Adam-it commented Dec 10, 2022

yep I still agree with my last comment 😅 @martinlingstuyl should we open this up?

@nicodecleyre
Copy link
Contributor

Pick me! 😄

@milanholemans
Copy link
Contributor

Awesome Nico, Thank you very much! The Issue is all yours

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

Successfully merging a pull request may close this issue.

7 participants