Adds 'pa environment get' script, solving #2109#2143
Adds 'pa environment get' script, solving #2109#2143appieschot wants to merge 1 commit intopnp:mainfrom
Conversation
waldekmastykarz
force-pushed
the
main
branch
5 times, most recently
from
9c69c74 to
35c16a0
Compare
plamber
left a comment
plamber
left a comment
There was a problem hiding this comment.
Hi @appieschot,
thank you for the contribution.
I reviewed the command and noticed few minor things that might require your attention.
When outputting the contents in "text" output mode I get a JSON in a properties object. Is there a way to reduce the output to the most important properties there and get the full output when using the JSON output?
Thank you
| const options: CommandOption[] = [ | ||
| { | ||
| option: '-n, --name <name>' | ||
| } | ||
| ]; |
There was a problem hiding this comment.
The name is required but the script is not giving me a warning if I do not specify a value.
I am receiving a 404 from the service
There was a problem hiding this comment.
Which command did you run exactly?
m365 pa environment getor
m365 pa environment get --nameIf the latter, then name is defined and its value is set to true
There was a problem hiding this comment.
Cant reproduce this issue on Windows or Linux other then with how @waldekmastykarz described it.
|
@plamber thanks for the review! Will pick up the points, will also create an issue for https://github.com/pnp/cli-microsoft365/blob/main/src/m365/flow/commands/environment/environment-get.ts as that contains the same issues ;-) Will pick those up as well. |
|
So regarding the output @plamber
I did the same implementation as the flow environment text output, see previous comment meaning everything. What do you feel should be important features to surface as text only? |
appieschot
force-pushed
the
feature/cmd-2109
branch
from
f08b74a to
81aa0a5
Compare
@appieschot: we had a similar discussion about the Yammer Search WebPart. Long story short we decided about some important properties to promote in the text output and keep the rest in the JSON output. To be honest, I do not have any preferences about which property should be promoted. Maybe you have some suggestions what might be appropriate to see immediately for every environment. It's your call. |
|
Since both commands are similar, I think it makes sense to keep them similar for now. If the output is still too long, let's align both of them as another iteration so that they are in sync. |
|
Lets keep this PR as is then, and ill create a new enhancement issue for both commands. |
waldekmastykarz
left a comment
waldekmastykarz
left a comment
There was a problem hiding this comment.
Very nicely done with two little things I've adjusted when merging the PR 👏
|
|
||
| ## Examples | ||
|
|
||
| Get information about the Microsoft Apps environment named _Default-d87a7535-dd31-4437-bfe1-95340acd55c5_ |
| @@ -0,0 +1,31 @@ | |||
| # pa environment ga | |||
|
Merged manually. Thank you! 👏 |
4 participants
Adds 'pa environment get' script, solving #2109