Open martinlingstuyl opened 10 months ago
Awesome idea that we should definitely follow up on to improve the user experience for folks on PowerShell. Let's dig deeper to see what's possible
This is something we definitely need to do. However, if we do this, it will be a breaking change, right?
Who knows... it should be optional and configurable after installation.
However, it could maybe lead to possible stuff that should be changed...and those could be breaking...
Updated the description with some research done on error handling. Also thought a bit about the prompt issues.
@pnp/cli-for-microsoft-365-maintainers : Curious to hear your opinions on the updated description and the proposal..
Folks use CLI in PowerShell both interactively and in scripts. We shouldn't assume that they use it only in scripts and should aim for it to be working in both types of usage. Changing the prompt to be sent to stderr seems like the right solution, even outside of PowerShell, so that it's not mixed with the actual command output.
agreed, let's investigate possible solutions
device code message should go to stderr indeed, either via ora or directly in CLI
I suggest that we integrate the necessary changes in the existing setup
command to avoid introducing yet another way of configuring the CLI.
Why would extending the setup command without introducing --integrationMode
be a breaking change?
Thanks for responding @waldekmastykarz!
Why would extending the setup command without introducing --integrationMode be a breaking change?
Scripts might break, because people already use ConvertFrom-Json
. That would mean the output is converted to json (by our central function) and then piped into ConvertFrom-Json
, which will cause exceptions.
I suggest that we integrate the necessary changes in the existing setup command to avoid introducing yet another way of configuring the CLI.
I agree that seems better. There is a downside in my optinion: updating the CLI will cause the functionality to be reset. You would need to rerun the setup command.
Also: what would you suggest: setting it by default when users are in PowerShell? Would it be an extra question? Should you be able to circumvent it? (I think you should)
Scripts might break, because people already use
ConvertFrom-Json
. That would mean the output is converted to json (by our central function) and then piped intoConvertFrom-Json
, which will cause exceptions.
Wouldn't the change only apply to our Invoke-CLIMicrosoft365Command
wrapper?
I agree that seems better. There is a downside in my optinion: updating the CLI will cause the functionality to be reset. You would need to rerun the setup command.
I think that this is reasonable: as we're adding new functionality, you might need to reconfigure it to get the latest features in your config setup.
Also: what would you suggest: setting it by default when users are in PowerShell? Would it be an extra question? Should you be able to circumvent it? (I think you should)
I think so. If we can verify then it's something that would reasonably be used by everyone in PowerShell, then I suggest we make it a part of the preset. If there's an exception, folks can always revert a particular setting to a different value.
Wouldn't the change only apply to our Invoke-CLIMicrosoft365Command wrapper?
The whole idea of that commandlet is that its automatically invoked from the m365.ps1 file...
So if I run:
m365 spo site list
What is returned are posh objects.
Writing the following code would then break:
m365 spo site list | ConvertFrom-Json
Hence it would be a breaking change to update the m365 setup command without some additional question or choice.
Any feedback on this one @pnp/cli-for-microsoft-365-maintainers, I know it's quite a read, but it's worth the effort π
Hence it would be a breaking change to update the m365 setup command without some additional question or choice.
Understood. Let's make it opt-in to avoid breaking people who have figured out using CLI without these proposed improvements.
I'd say, let's make it opt-in for now and make it the default behavior in v8
.
@waldekmastykarz, @milanholemans
m365 setup --scripting --usePowerShellIntegration
m365 setup --scripting --optInPowerShellIntegration
Or interactively:
m365 setup
> question 1
> question 2
> Do you want to opt in to using our experimental PowerShell Integration mode?
Additionally: do you have feedback on challenge 2?
Nice job with all this research already @martinlingstuyl. We're definitely missing some integration with our most commonly used shell. Using | Convertfrom-Json
is something that is very frustrating to use in scripts. It works most of the time but on occasions, it tends to fail but then you've to convert it to a hash table object. And let's not get started on error handling. That's just a pain.
Regarding your suggestion of enabling integration mode. That would be quite a nice feature out-of-the-block but it would indeed be a breaking change. So I'm also all for adding it as an additional option with m365 setup
and with the next mayor making it default. My vote for the option name would be --usePowerShellIntegration
. For interactive mode, what about > Do you want to use PowerShell Integration mode?
and then clarify this question more in the docs.
Additionally: do you have feedback on challenge 2?
βοΈ and this one @Jwaegebaert, @waldekmastykarz, @milanholemans?
it tends to fail but then you've to convert it to a hash table object.
By the way: I believe we fixed all occurrences of this. It was due to duplicate properties Id
and ID
on listitem endpoints.
The variable contents will be converted from json. If the conversion fails, the raw output is returned. (which will mean text, csv and md modes can still be used)
Couldn't we just check what is the expected output mode and not convert to the variable if it's other than JSON?
I'd say, let's make it opt-in for now and make it the default behavior in v8.
I agree with @milanholemans on this oneπ
Challenge 1
Seems ok but
Solution: Pipe the prompt to stderr by configuring inquirer. Also move the device code message to the ora-spinner, which circumvents the stdout stream.
I think we already discussed the device code needs to end up together with prompts in stderr π€. Otherwise it is not possible to catch it when using CLI as API in a node.js app
Challenge 2
Seems legit ππ. Maybe we could name the setting promptMessageOutput
? Since (if I understand it properly) it's only meant to change the behavior of prompts
Challenge 3
The missing error message color does not seem a problem for me π as long as the error is part of stderr
Challenge 4
This is a bummer π€. Won't setting $ErrorActionPreference to continue overwrite what is set by the user in the script?
So if someone has it set to stop
if we now set it to continue in our m365-invoke.ps1 (if I understand your proposal properly π) won't it like overwrite the default setting?
BTW. Sorry for the late reply π Awesome job on setting this all up. You rock π€©
Seems legit ππ. Maybe we could name the setting
promptMessageOutput
? Since (if I understand it properly) it's only meant to change the behavior of prompts
It's not prompts here. The point is we want to redirect error messages without redirecting other verbose content like debug logs, verbose logs, prompts and spinners. which is why I proposed errorMessageOutput
and verboseMessageOutput
. debug & verbose logs, prompts and spinners would then fall under the heading of verboseMessageOutput
.
That logical?
This is a bummer π€. Won't setting $ErrorActionPreference to continue overwrite what is set by the user in the script? So if someone has it set to
stop
if we now set it to continue in our m365-invoke.ps1 (if I understand your proposal properly π) won't it like overwrite the default setting?
My idea would be to throw
the error if the user has configured $ErrorActionPreference
as Stop
and to Write-Error
the error if the user has configured $ErrorActionPreference
as Continue
. So we don't overwrite $ErrorActionPreference
. We decide what to do with the error based on the users preference. Also, we don't have the -ErrorAction
option, so errors can only be influenced by using $ErrorActionPreference
.
Sorry for the late reply π
Never mind, thanks for taking the time to read and comment!
Any other opinions? @pnp/cli-for-microsoft-365-maintainers
It's not prompts here. The point is we want to redirect error messages without redirecting other verbose content like debug logs, verbose logs, prompts and spinners. which is why I proposed
errorMessageOutput
andverboseMessageOutput
. debug & verbose logs, prompts and spinners would then fall under the heading ofverboseMessageOutput
.That logical?
aaaa ok. clear. Yes, that is logical π
My idea would be to
throw
the error if the user has configured$ErrorActionPreference
asStop
and toWrite-Error
the error if the user has configured$ErrorActionPreference
asContinue
. So we don't overwrite$ErrorActionPreference
. We decide what to do with the error based on the users preference. Also, we don't have the-ErrorAction
option, so errors can only be influenced by using$ErrorActionPreference
.
ok clear now as well π
It's a bit of a bummer that we'll lose the colors (Challenge 3). If I'm not mistaken, this will mean by default that errors won't be shown in red. I think we better look for a solution regarding this one as we don't have a lot of visual pointers in a CLI so losing our color somewhere would be a big plus that we'll lose.
The color of the error should still be red, because we'll write it ourselves after the CLI throws it and the invoke function intercepts it, @Jwaegebaert.
But I agree with you that loosing colors is indeed a bummer.
Of course, we also get new colors, because PowerShell colorizes the response objects.
Yeah, good point. Lost sight of that one. Then I've nothing further to add π
Ok, so the issue is twofold:
1) Posh objects: everybody who uses m365 in PowerShell often uses
| Convertfrom-Json
to convert the output of the cli to objects for use in scripting. 2) Error handling: the CLI does not work like PowerShell commandlets. PowerShell has some interesting features here, like being able to configure what a script should do when it runs into an error. (-ErrorAction
and $ErrorActionPreference There are some differences between how PowerShell interacts with lowerlying commandline tools. All in all this means that when building a solid script, you should write your own code to do some correct error handling.We'd like to improve the general e2e flow of PowerShell users in these two regards.
π‘ Idea
The idea would be to have better integration with PowerShell. By inserting a PowerShell function and piping command output in for processing before returning to the user.
Checkout the screenshot to see how I did that manually and what it causes:
π Functional requirements
1. PowerShell object output
2. Exception handling
$ErrorActionPreference
: CLI should throw a terminating exception if the preference is configured as βStopβ, it should write a non-terminating exception when the preference is set to βContinueβ, etc. Hence you should be able to catch an error in scripts using βtry catchβ statements.3. Configuration and Usability
βοΈ Implementation details
1. Enabling the integration mode
After installing the CLI, the m365.ps1 file will need to be adapted. This should be done by running the m365 setup command.
An extra question should currently decide if integration mode is wanted. For example: 'Do you want the CLI to return PowerShell objects?' This question is only asked if the shell you are using is PowerShell.
...or using presets with a flag
The reason for the extra flag and the extra question is that existing scripts where people already use
ConvertFrom-Json
would break. Unless we decide that is not a breaking change.2. What happens when the user enables the integration mode
Invoke-M365
commandlet.3. JSON conversion in the
Invoke-M365
commandlet4. Exception handling and the integration mode
$ErrorActionPreference
and$PSNativeCommandUseErrorActionPreference
variables should be cached in the m365.ps1 file and set toContinue
/$false
respectively. (See challenge 2.3 for the reaon)Invoke-M365
commandlet expects an error in the stdout output stream. (Config keyerrorOutput
=stdout
)Invoke-M365
commandlet expects the error in JSON form. (Config keyprintErrorsAsPlainText
=false
)Invoke-M365
commandlet writes the error or throws the error, taking the cached$ErrorActionPreference
into account.$ErrorActionPreference
and$PSNativeCommandUseErrorActionPreference
variables are restored to their original values.π¬ Scenario's
1. When using CLI scripting in non-interactive environments
When using the CLI in a non-interactive environment. (For example: Azure PowerShell Functions), users can run the following line of code before doing anything else:
This command will currently configure all config keys in the correct way.
Using the new PowerShell integration, this can be kept as-is. (Aside from the extra flag
--usePowerShellIntegration
) the configuration is exactly the same. The only thing that's adapted is the m365.ps1 file. Any errors will be written to the stdout stream, and this will be picked up by our PowerShell integration.2. When using the CLI in interactive environments
When using the CLI in an interactive environment (the terminal), there is an issue with requirement 2.1. For the integration to work we'll need errors to be written to the stdout stream in JSON form. We'll need to configure error output to the stdout stream, but this will cause all prompts to be written there as well, freezing any interaction for the user. (See challenge 1).
πΊ Challenge 1: The CLI Prompt and device code login cause the
Invoke-M365CLICommand
to 'freeze'.Read the details
The prompt in the CLI is run bij inquirer during the execution of the command. This poses a problem. If we run the command using `$output = $input`, the `$output` variable will only be assigned a value after the `$input` has run. In practice this means the prompts will not show to the user, and the CLI seems to freeze. There seem to be two solutions: 1. Disable the prompt in this 'PowerShell integration mode'. We could decide this is just not working well together, and that you would typically use this in scripts, not in interactive mode. 2. Change the prompt to use `stderr`. This should be possible according to the inquirer docs. The login by device code flow has the same problem as the prompt issue... The login command waits for the user to use the device code. But the device code has not been printed to the screen because the $output-variable is not yet assigned. This poses a problem...Solution: Pipe the prompt to stderr by configuring inquirer. Also move the device code message to the ora-spinner, which circumvents the stdout stream.
πΊ Challenge 2: Native commands (like m365 cli) do not honour PowerShell ErrorAction preferences.
Read the details
In PowerShell there is a variable that controls what you would like failing commandlets to cause. It's called `$ErrorActionPreference`. By default it's set to 'Continue', which means that commandlets that fail will not stop the script execution. The script will continue execution of following lines. This is annoying behavior in scripts, where you sometimes would like Exceptions to be stopping further execution of your script. This is why in PowerShell I often set `$ErrorActionPreference = 'Stop'`. However: Exceptions thrown in in Native commands that are called by PowerShell (cmd files, exe files, bat files, like the CLI for Microsoft 365) do not work well with this currently. In PowerShell 7.3 there is an experimental feature: [$PSNativeCommandUseErrorActionPreference](https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_preference_variables?view=powershell-7.3#psnativecommanduseerroractionpreference) Setting this to true will force the CLI to follow `$ErrorActionPreference`. Errors will be thrown, _but_ the error text is currently not visible in the thrown error. In PowerShell 7.2 the feature is not present. Scripts will just continue on.In short: We need to handle exceptions ourselves to work around the (current) drawbacks of PowerShell. To do that, exceptions need to be written to stdout (as json objects). However: When
errorOutput
is set to stdout, ALL output goes there, including prompts and the login spinner, meaning we're again tackled by Challenge 1.Potential solution: We may to change config keys a bit: need an extra config key to just send the error to
stdout
and leave the rest instderr
. Proposal:errorOutput
stderr
errorMessageOutput
stderr
stdout
to write error messages to the output stream.verboseMessageOutput
stderr
stdout
to write verbose messages to the output stream.πΊ Challenge 3: Colors in the error stream are lost
When the CLI command output is piped into a commandlet, the error stream is redirected in some way as well, losing all colorization in the process.
Solution: I've not yet found a solution. Maybe we ought just to accept this side-effect. As it's not really important.
πΊ Challenge 4: Clean errors without stacktrace
Read the details
There are downsides to throwing or writing errors in PowerShell. An ugly error with a useless stacktrace will show, because the command is called from a script file (m365.ps1), which is not what we would want. Clean errors without stacktrace seem hard to get. ![image](https://github.com/pnp/cli-microsoft365/assets/5267487/c46ce3e5-c105-4984-bfe1-12cb05267251) ![image](https://github.com/pnp/cli-microsoft365/assets/5267487/15945717-cea7-4fd5-90d5-8af94ac7972a) There are some ideas on how to get clean errors though: [how to get clean errors without stacktrace](https://stackoverflow.com/questions/38064704/how-can-i-display-a-naked-error-message-in-powershell-without-an-accompanying) We'd need some research into what would be an optimal solution.Potential solution: It seems to be possible to write a clean error when
$ErrorActionPreference
is set toContinue
. When throwing an error, we'll always see the stacktrace.β Tasks
Invoke-M365
commandletinvoke-m365
commandlet and updating the m365.ps1 file.