Adds command pa app permission list
#5166
Conversation
|
Thank you, will try to review it ASAP |
| permissions.forEach(permission => { | ||
| permission.roleName = permission.properties.roleName; | ||
| permission.principalId = permission.properties.principal.id; | ||
| permission.principalType = permission.properties.principal.type; | ||
| }); |
There was a problem hiding this comment.
Let's do this only when it's really needed (when the output should be trimmed).
There was a problem hiding this comment.
I was wondering, what are you meaning exactly with when the output should be trimmed.
Currently, we have the method Cli.shouldTrimOutput, which will only trim the output when we use --output text, but that results in the following when using --output md or --output csv
In this case, wouldn't it be better if we do args.options.output !== json, then the output would be like this:
There was a problem hiding this comment.
shouldTrimOutput was made so we didn't have to check each command separately when we had to modify the result. And I kinda agree that the CSV and MD output for this command is ... not so ideal indeed.
@pnp/cli-for-microsoft-365-maintainers what is the way to go right now? Just use Cli.shouldTrimOutput(...) and leave the CSV and MD output scarce as shown above?
@martinlingstuyl I know you changed some stuff about the output. Maybe you could clarify it a bit?
There was a problem hiding this comment.
Hi @milanholemans, @MathijsVerbeeck, we changed two things about the output:
- All columns should be shown in md and csv output. (They were trimmed before)
- Do not show nested objects in csv and md uitput, unless expressly requested using jmespath.
In this scenario, the question is what you would want to show; if the nested properties are really important to always show, and you don't want to change the api responses in json mode, you could of course flatten the object when output !== 'json', like Mathijs suggests.
There was a problem hiding this comment.
So that means that the function Cli.shouldTrimOutput(...) has no use anymore?
There was a problem hiding this comment.
I'm not sure if you can say that without looking at where it's used. There may be valid use cases.
There was a problem hiding this comment.
But in this instance at least, you don't need it.
There was a problem hiding this comment.
I'm not sure if you can say that without looking at where it's used. There may be valid use cases.
At this point I think it has outlived its usefulness unless we add csv and md again.
|
Hi Milan Thanks for the review. I've changed your remarks (and left a comment at two remarks). Could you, if you have no further questions or remarks, have another look? Of we change our decision regarding the trimming of the output, I'll obviously change it 😉 |
There was a problem hiding this comment.
We're almost there @MathijsVerbeeck, let's do a couple more changes before we merge it.
|
Merged manually. Thank you for this awesome new command! 👏 |
Closes #4645