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

Enhancement: List all properties returned by Graph when using m365 aad user list #4667

Closed
ValerasNarbutas opened this issue Mar 20, 2023 · 17 comments

Comments

@ValerasNarbutas
Copy link
Contributor

ValerasNarbutas commented Mar 20, 2023

Idea

Currently this command "m365 aad user list" return only userprincipalname and display name, would be great if it could return same number properties as graph API does by default.

Additional Info

one suggestion:
from @Adam-it
we could just modify this

const properties: string[] = args.options.properties ?
args.options.properties.split(',').map(p => p.trim()) :
['userPrincipalName', 'displayName'];

not to trim to ['userPrincipalName', 'displayName'];

@milanholemans
Copy link
Contributor

milanholemans commented Mar 20, 2023

Great suggestion @ValerasNarbutas. To me it doesn't make much sense that we aren't returning all properties we retrieve from Graph.
My suggestion would be, instead of introducing a new option, let's just return all properties from Graph when running m365 aad user list (so when no --properties option is passed).

@ValerasNarbutas
Copy link
Contributor Author

ValerasNarbutas commented Mar 20, 2023

Great suggestion @ValerasNarbutas. To me it doesn't make much sense that we aren't returning all properties we retrieve from Graph. My suggestion would be, instead of introducing a new option, let's just return all properties from Graph when running m365 aad user list (so when no --properties option is passed).

totally agree, this is my first command example.. i have to do 2 examples as per requirement :D. A default return would be just enough.

@milanholemans
Copy link
Contributor

totally agree, this is my first command example.. i have to do 2 examples as per requirement :D. A default return would be just enough.

Yes that's because you used a new command issue template, while this isn't a new command but rather an enhancement. We don't have an enhancement template because it is quite hard to put it in a fixed template. Therefore you should actually use a blank issue to write down enhancements.

So for this enhancement, I don't think we need extra examples since we are only extending something that already exist.

@ValerasNarbutas
Copy link
Contributor Author

So for this enhancement,

ahh, thanks @milanholemans , i will do this next time!

@milanholemans
Copy link
Contributor

I've trimmed down your issue a bit (removed the unnecessary sections). If this looks good for you, we can start implementing it.

@milanholemans milanholemans changed the title New command: m365 aad user list --properties all Enhancement: List all properties returned by Graph when using m365 aad user list Mar 20, 2023
@ValerasNarbutas
Copy link
Contributor Author

I've trimmed down your issue a bit (removed the unnecessary sections). If this looks good for you, we can start implementing it.

nothing to add, thanks @milanholemans

@milanholemans
Copy link
Contributor

Just to give some extra context, this is not a breaking change because we will just enrich the output with extra properties that are returned by default by Graph. The already existing properties will still be there.

@Jwaegebaert
Copy link
Contributor

Jwaegebaert commented Mar 21, 2023

Nice enhancement @ValerasNarbutas! It's indeed more logical that the command would return all the available properties instead of trimming it.

Do you want to work on this issue?

@ValerasNarbutas
Copy link
Contributor Author

Nice enhancement @ValerasNarbutas! It's indeed more logical that the command would return all the available properties instead of trimming it.

Do you want to work on this issue?

with pleasure :) @Jwaegebaert
i just need my previous pull request merged, the one for typos.

@Jwaegebaert
Copy link
Contributor

Awesome! The issue is all yours 🚀

@milanholemans
Copy link
Contributor

milanholemans commented Mar 21, 2023

with pleasure :) @Jwaegebaert i just need my previous pull request merged, the one for typos.

One small tip: when contributing to GitHub repositories like ours, the best strategy is that you create a new git branch based on your main branch, for each issue you are working on, and put your contributions for that specific issue in there. That makes sure that all your contributions are isolated on a specific branch and your main branch remains untouched (as it should be because this one should be in sync with our main).
This also enables you to contribute to multiple issues at without code from issue A affecting code from issue B.

Currently you cannot do this because you already made a PR with changes on your main branch. But keep this in mind for the future.

@ValerasNarbutas
Copy link
Contributor Author

is also enables you to contribute to multiple issues at without

this is exactly tip i needed !! @milanholemans i had it somewhere back in my mind, but never tried :)
thanks.

@Jwaegebaert
Copy link
Contributor

Hey @ValerasNarbutas, just checking in. How is everything going regarding this issue?

@waldekmastykarz
Copy link
Member

Resetting due to lack of response

@MathijsVerbeeck
Copy link
Contributor

Can I work on this?

@milanholemans
Copy link
Contributor

@MathijsVerbeeck note that this might interfere with #5644

@MathijsVerbeeck
Copy link
Contributor

I'll wait for the merge of Nandeeps' PR and work on this afterwards.

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.

5 participants