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

CLI stuck on prompt when running in non-interactive mode #142

Closed
waldekmastykarz opened this issue Dec 21, 2017 · 10 comments
Closed

CLI stuck on prompt when running in non-interactive mode #142

waldekmastykarz opened this issue Dec 21, 2017 · 10 comments
Assignees
Milestone

Comments

@waldekmastykarz
Copy link
Member

When executing a command that yields a prompt, such as spo app retract, when executed in non-interactive mode, the CLI doesn't render the prompt in the shell. Instead it hangs. If the prompt is suppressed using the --confirm option, the command works as expected. Verify if this is a limitation in Vorpal or a bug in the CLI implementation.

@waldekmastykarz
Copy link
Member Author

After a quick look it seems like an issue with Vorpal. There was an issue with PR logged for it in the past (dthree/vorpal#166) but apparently there were some side-effects which is why it isn't available in the current build of Vorpal. Let's have a closer look and see if we can find a way to deal with this issue in the CLI.

@waldekmastykarz waldekmastykarz added bug needs peer review Needs second pair of eyes to review the spec or PR and removed question labels Dec 31, 2017
waldekmastykarz added a commit to waldekmastykarz/cli-microsoft365 that referenced this issue Dec 31, 2017
@waldekmastykarz
Copy link
Member Author

I've got a workaround for the issue in Vorpal. The only side effect is an extra empty line printed before and after the command output, but they seem harmless and acceptable trade-off to be able to correctly display prompts in the non-interactive mode in the CLI.

I would appreciate it, if you could have a look at the fix guys, give it a test and tell me if I missed anything and if it would be acceptable to merge in the CLI.

See the fix in my fork https://github.com/waldekmastykarz/office365-cli/tree/fix-noninteractiveprompt.

/cc: @VelinGeorgiev @andrewconnell

@VelinGeorgiev
Copy link
Contributor

Hey @waldekmastykarz,

Tested on Windows and works as expected. Attached are Windows screens:
cmd.exe:
cmd exe
cmder:
cmder

I possess poor UI skills, but I personally do not care if it would print more empty lines. After all, one cmd/bash screen is ugly by design and this would not make big difference :). I would not pay attention if you did not mention it here.

@waldekmastykarz
Copy link
Member Author

Thanks for checking @VelinGeorgiev. The result is similar (if not the same) as on macOS

@waldekmastykarz
Copy link
Member Author

waldekmastykarz commented Jan 5, 2018

I've done some additional testing and it seems like the additional empty lines lead to errors when trying to process the JSON output. I tried using both jq in sh and ConvertFrom-Json in PowerShell. While the errors are slightly different, you do get an error which is specifically related to trailing empty line. We can't merge this fix in its current state as it would block users from building scripts using the CLI.

It seems like Vorpal is adding more than just an empty line to the output. Here are the chars I'm seeing:

^[[1D^[[1C^[[1000D^[[K ^[[1D^[[1C\n[{"Title":"....

And it's exactly these first characters that trip parsing JSON. We could of course remove them using grep or something similar but it would unnecessarily complicate using the CLI.

@waldekmastykarz waldekmastykarz removed the needs peer review Needs second pair of eyes to review the spec or PR label Jan 5, 2018
waldekmastykarz added a commit to waldekmastykarz/cli-microsoft365 that referenced this issue Jan 5, 2018
@waldekmastykarz
Copy link
Member Author

I have a working solution based on a fix applied to a forked Vorpal repo which you can see at https://github.com/waldekmastykarz/office365-cli/tree/fix-noninteractiveprompt-vorpal (notice the vorpal dep in package.json pointing to my forked repo with the fix). Ideally, we'd reference Vorpal from npm, but given the fix for our issue has been waiting for almost a year, I suspect chances are remote we will see it merged in the main repo any time soon. If it does happen, we can always flip back to the main repo.

What do you think @andrewconnell @VelinGeorgiev?

@VelinGeorgiev
Copy link
Contributor

VelinGeorgiev commented Jan 8, 2018

Agree @waldekmastykarz, this should be the way to go. I will have a look at that within the next few days.

@waldekmastykarz
Copy link
Member Author

Rather than looking at the fork, have a look at the PR @VelinGeorgiev. I'm also planning to merge all PRs in a couple of days so we can give them a try in a beta version.

@VelinGeorgiev
Copy link
Contributor

@waldekmastykarz, this was in my todo list. I tested it and it works as expected in non-interactive mode with both --confirm flag specified and without.
image

@waldekmastykarz
Copy link
Member Author

Sweet! Thank you for another pair of eyes and confirming it's working as expected!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants