pnp / cli-microsoft365

Manage Microsoft 365 and SharePoint Framework projects on any platform
https://aka.ms/cli-m365
MIT License
907 stars 317 forks source link

Extend `m365 setup` command to setup command completion #5730

Open Adam-it opened 8 months ago

Adam-it commented 8 months ago

I was thinking that when the user runs m365 setup command and picks interactively as the first option we could add an additional question/option to configure command completion for the user if possible.

Currently running the command with those options looks like image

When 'interactively' was picked I propose adding an additional step/question at the end like Would you like to set up command completion? (Y/n)

Picking Y would setup the command completion as described in our spec https://pnp.github.io/cli-microsoft365/user-guide/completion#powershell. The logic behind it should o check the user shell and if possible should execute the m365 cli completion command for the appropriate shell.

We should also check if the user is running CLI in any of the supported shells and only then show this additional question.

When PowerShell is used will need to do additional hack in order to retrieve the user $profile.CurrentUserAllHosts and pass it down to the command as PowerShell requires the profile when executed m365 cli completion pwsh setup --profile $profile. In order to retrieve the $profile.CurrentUserAllHosts we may do something like

exec(`powershell.exe -Command "${command}"`, (error, stdout, stderr) => {

and in place of the command do a Write-Ouput $profile.CurrentUserAllHosts. We need to check if user is using windows PowerShell or pwsh.

martinlingstuyl commented 8 months ago

Totally agree on this @Adam-it.

In the cli completion pwsh setup command, we ask for a profile path, which (in PowerShell) is available in the $profile variable. Do you know if we specifically need to prompt for it to know where the profile file is located?

Adam-it commented 8 months ago

Totally agree on this @Adam-it.

In the cli completion pwsh setup command, we ask for a profile path, which (in PowerShell) is available in the $profile variable. Do you know if we specifically need to prompt for it to know where the profile file is located?

Nope. But I will recheck

Adam-it commented 8 months ago

@martinlingstuyl so it seems for the CLI setup we need to somehow know where is the PS profile and we may not assume the default path. Getting this env PS variable from withing node.js app is not possible. What I investigated is a bit of a hack. We could have a helper ps1 script which would print the $profile and then in the CLI command execute it as a child process like

exec('powershell.exe -File getProfileHelper.ps1', (error, stdout, stderr) => {
    if (error) {
        console.error(`Execution error: ${error}`);
        return;
    }

    console.log(`our profile path => ${stdout}`);
});

if it will not fail we may assume what we get in the stdout of this script will be the path coming from the profile. We could do that only for the setup. What is your opinion on that? TBH even if it is possible it is very hacky IMO and I would not recommend it. Instead, I would leave it as is and for powershell just provide an additional prompt for the user to pass the $profile. @pnp/cli-for-microsoft-365-maintainers any other feedback on that? Or maybe someone has a better idea how to obtain that path?

waldekmastykarz commented 8 months ago

We should go a step further and don't make assumptions about $profile. While working on Dev Proxy I learned that there are multiple profiles, and if you configure some settings with $profile in the terminal, they're not picked up by the VSCode extension terminal, because it's got its own profile. So instead, you should use $profile.CurrentUserAllHosts, but perhaps folks have different preferences based on their expertise level

waldekmastykarz commented 8 months ago

I'd also argue, that command completion is not just for beginners, so it should be configured when you choose to use CLI interactively.

Adam-it commented 8 months ago

I'd also argue, that command completion is not just for beginners, so it should be configured when you choose to use CLI interactively.

Good idea, we may adjust to ask for it after someone picked interactively. What do you think of the hack I proposed 😅?

waldekmastykarz commented 7 months ago

I like the idea of detecting the profile path. Do we need a whole script for it or could we simply get the output of the $profile.CurrentUserAllHosts variable? Also, we need to make it work x-plat so not just default to powershell.exe but also consider pwsh for non-Windows OSes.

Adam-it commented 6 months ago

@waldekmastykarz, @martinlingstuyl thanks for your feedback. I aligned the spec to move this issue a bit forward. As for

I like the idea of detecting the profile path. Do we need a whole script for it...

Nope, I think we may just do something like

exec(`powershell.exe -Command "${command}"`, (error, stdout, stderr) => { ...... 

and in place of the command do a Write-Ouput $profile. 'Should' work 😉.

or could we simply get the output of the $profile.CurrentUserAllHosts variable?

.. hmm not sure. I am a bit to stupid to answer this question I guess 🤔. My idea was to reuse the m365 cli completion pwsh setup --profile $profile command which takes the $profile so I was thinking of just retrieving it from PS and passing it over to the command under the hood. Not sure if this approach would work if we would just retrieve $profile.CurrentUserAllHosts.

Also, we need to make it work x-plat so not just default to powershell.exe but also consider pwsh for non-Windows OSes.

good point. Do you remember if anywhere else CLI already checks for the OS? we could use windows powershell for windows and pwsh for other right?

waldekmastykarz commented 6 months ago

we could use windows powershell for windows and pwsh for other right?

I think that's dangerous. More and more folks use Microsoft PowerShell (pwsh) on Windows. We definitely shouldn't make any assumptions here.

Adam-it commented 6 months ago

yes thats true. I wonder if we may somehow check if pwsh is available 🤔 before we execute the step to get the profile. Need to research that

waldekmastykarz commented 6 months ago

yes thats true. I wonder if we may somehow check if pwsh is available 🤔 before we execute the step to get the profile. Need to research that

I'd say that we shouldn't just look if it's available but rather check which shell we're being run from and configure it for that shell.

Adam-it commented 6 months ago

I'd say that we shouldn't just look if it's available but rather check which shell we're being run from and configure it for that shell.

Exactly, and only for PowerShell we will need this hack with getting the profile 👍. Ok I will update the spec 👍

Adam-it commented 6 months ago

ok @waldekmastykarz, @martinlingstuyl I updated the spec to add more steps and info what we should do for PowerShell setup. From my quick research if we retrieve it in this way and pass it over to execute the command completion for PowerShell it should work, as it is actually just retrieving and passing over the path to the profile.

@pnp/cli-for-microsoft-365-maintainers any other feedback or is it good enough to open it up?

martinlingstuyl commented 6 months ago

One remark: I still think it's best if we use $profile.CurrentUserAllHosts, instead of just $profile, the last one will only add the reference to the profile file of the current host (vs code for example) this means that autocomplete will then only work within the vscode terminal, while the setup is global. CurrentUserAllHosts is just another path to the profile file that is loaded in all terminal hosts.

Adam-it commented 6 months ago

One remark: I still think it's best if we use $profile.CurrentUserAllHosts, instead of just $profile, the last one will only add the reference to the profile file of the current host (vs code for example) this means that autocomplete will then only work within the vscode terminal, while the setup is global. CurrentUserAllHosts is just another path to the profile file that is loaded in all terminal hosts.

I did not know that 🤦‍♂️😅. Awesome suggestion 👍 I updated the spec

Adam-it commented 6 months ago

ok then. lets open this up 👍