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

Adds "aad group user list" command. Closes #5469 #5537

Closed
wants to merge 9 commits into from

Conversation

nanddeepn
Copy link
Contributor

Adds aad group user list command. Closes #5469

😍 Happy Hacktoberfest !

@Jwaegebaert
Copy link
Contributor

Hey @nanddeepn, looks like the docs job is failing. Could you take a look at that?

@Jwaegebaert Jwaegebaert added the hacktoberfest-accepted Accept for hacktoberfest, will merge later label Oct 2, 2023
@nanddeepn
Copy link
Contributor Author

nanddeepn commented Oct 2, 2023

Hi @Jwaegebaert
Any guidance, how can I reproduce this locally?
May be this can help? https://pnp.github.io/cli-microsoft365/contribute/new-command/writing-the-docs/#running-docusaurus-locally

@Jwaegebaert
Copy link
Contributor

Yes, indeed. By following the link you posted you should be able to reproduce it locally

@Jwaegebaert Jwaegebaert removed the hacktoberfest-accepted Accept for hacktoberfest, will merge later label Oct 2, 2023
@nanddeepn
Copy link
Contributor Author

Hi @Jwaegebaert
I tried the instructions locally and the documentation seems to be working fine.

image

Anything else, I am missing here?

@nanddeepn
Copy link
Contributor Author

docs job fails, when I add below to sidebars.js. Any suggestions please?

{
     type: 'doc',
     label: 'group user list',
     id: 'cmd/aad/group/group-user-list'
}

@Jwaegebaert
Copy link
Contributor

Jwaegebaert commented Oct 3, 2023

@nanddeepn, we're looking into it. It seems to be more complex than we imagined. I'll keep you posted once we find something.

Copy link
Contributor

@martinlingstuyl martinlingstuyl left a comment

Choose a reason for hiding this comment

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

Hi @nanddeepn, thanks for your contribution. I did a review and have some comments. I also have a point of open discussion I'm not yet sure about. I'll ping Milan about it.

docs/docs/cmd/aad/group/group-user-list.mdx Outdated Show resolved Hide resolved
src/m365/aad/commands/group/group-user-list.spec.ts Outdated Show resolved Hide resolved
src/m365/aad/commands/group/group-user-list.ts Outdated Show resolved Hide resolved
@martinlingstuyl martinlingstuyl self-assigned this Oct 3, 2023
@martinlingstuyl
Copy link
Contributor

Hi @nanddeepn, I've discussed this internally, and it seems we've got an interesting confusion here. 'Owners and Members' are something else then 'Members and Guests', the userType property is an example of the last. MS graph returns user objects, and userType is simply a user property there.

It's the fault of the specs, so we'll be changing the specs. In its current state it's just not feasible and confusing.

We've seen that this code is a copy of m365group user list, so that command will need to change as well (later on)

You'll see it when the specs have been updated.

@martinlingstuyl martinlingstuyl marked this pull request as draft October 3, 2023 18:37
@nanddeepn nanddeepn marked this pull request as ready for review October 4, 2023 10:48
@martinlingstuyl
Copy link
Contributor

He @nanddeepn, the specs have not yet been updated, 😂 you'll have to wait some more..

@milanholemans
Copy link
Contributor

Hi @nanddeepn

We took a few days to look at the issue you are facing with Docusaurus.

In a nutshell:

We managed to get it working again by adding an extra config value: trailingSlash: false. This should be added in the Docusaurus config file here:

title: 'CLI for Microsoft 365',
titleDelimiter: '-',
tagline: 'Docs',
url: 'https://pnp.github.io',
baseUrl: '/cli-microsoft365/',
onBrokenLinks: 'throw',
onBrokenMarkdownLinks: 'throw',
favicon: 'img/favicon.ico',
organizationName: 'pnp',
projectName: 'cli-microsoft365',

Why does this make the pipeline work? We have no idea, to be honest. In fact, we even reached out to the mods/maintainers of Docusaurus, and even they have no idea what caused this issue and why the trailingSlash is fixing it.
As far as we know, this setting can't hurt us, so we'll take it as a solution. Could you add it to this PR?

Thanks!

@milanholemans milanholemans marked this pull request as draft October 8, 2023 20:06
@nanddeepn nanddeepn marked this pull request as ready for review October 11, 2023 11:49
Copy link
Contributor

@martinlingstuyl martinlingstuyl left a comment

Choose a reason for hiding this comment

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

Hi @nanddeepn, let's look at some comments before we merge this...

docs/docs/cmd/aad/group/group-user-list.mdx Outdated Show resolved Hide resolved
docs/docs/cmd/aad/group/group-user-list.mdx Show resolved Hide resolved
src/utils/odata.ts Show resolved Hide resolved
src/m365/aad/commands/group/group-user-list.ts Outdated Show resolved Hide resolved
src/m365/aad/commands/group/group-user-list.ts Outdated Show resolved Hide resolved
src/m365/aad/commands/group/group-user-list.ts Outdated Show resolved Hide resolved
src/m365/aad/commands/group/group-user-list.ts Outdated Show resolved Hide resolved
src/m365/aad/commands/group/group-user-list.ts Outdated Show resolved Hide resolved
@martinlingstuyl martinlingstuyl marked this pull request as draft October 12, 2023 20:56
@nanddeepn nanddeepn marked this pull request as ready for review October 13, 2023 11:24
Copy link
Contributor

@martinlingstuyl martinlingstuyl left a comment

Choose a reason for hiding this comment

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

Hi @nanddeepn, I've got a few last round comments. We'll merge this as soon as possible afterwards.

src/m365/aad/commands/group/group-user-list.ts Outdated Show resolved Hide resolved
src/m365/aad/commands/group/group-user-list.ts Outdated Show resolved Hide resolved
src/m365/aad/commands/group/group-user-list.ts Outdated Show resolved Hide resolved
@martinlingstuyl martinlingstuyl marked this pull request as draft October 18, 2023 15:07
@nanddeepn nanddeepn marked this pull request as ready for review October 19, 2023 11:34
Copy link
Contributor

@martinlingstuyl martinlingstuyl left a comment

Choose a reason for hiding this comment

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

Looks fine with some last cosmetic changes

List all group users from a group specified by name. For each one return the display name, e-mail address, and memberof.

```sh
m365 aad group user list --groupDisplayName Developers --properties "displayName,mail,memberof/id"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use manager/displayName as example.

Let's include an extra example with manager/*

@martinlingstuyl
Copy link
Contributor

Merged manually, thanks 👏

@martinlingstuyl martinlingstuyl added the hacktoberfest-accepted Accept for hacktoberfest, will merge later label Oct 25, 2023
@nanddeepn nanddeepn deleted the issue-5469 branch October 25, 2023 06:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Accept for hacktoberfest, will merge later pr-merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New command: aad group user list
4 participants