search

pnp / cli-microsoft365

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

Bug report: CLI does not work in Azure Cloud Shell #6117

Closed Adam-it closed 2 months ago

Adam-it commented 3 months ago

Priority

(Urgent) I can't use the CLI

Description

When executing any CLI command we don't see any output.

Steps to reproduce

  1. login to Azure and open Cloud shell
  2. run m365 version

Expected results

should get any CLI command response or see any effect. For example for m365 version we should see version and we don't get anything

Actual results

nothing happens, no error and no output

Diagnostics

No response

CLI for Microsoft 365 version

7.10.0

nodejs version

18.20.2

Operating system (environment)

Azure Cloud Shell

Shell

PowerShell

cli doctor

No response

Additional Info

No response

Adam-it commented 3 months ago

when investigating this I found out this actually stopped working on 9.12.2023 at this 7.3.0-beta.ca3f7f5": "2023-12-09T18:40:15.574Z beta release image

This release is connected to a single PR with this change https://github.com/pnp/cli-microsoft365/commit/10835b8b81d52a981ed8ee3e47ed2484e2266405

I am investigating why this broke CLI only in AZ Cloud Shell

Adam-it commented 3 months ago

Ok I found the problem. It is actually very strange 🤦‍♂️ so when checking the updates we did between 7.2 and 7.3 

    7.2.0": "2023-11-30T08:32:20.277Z",
    "7.3.0-beta.73848f3": "2023-11-30T08:42:46.581Z",
    "7.3.0-beta.a98760b": "2023-11-30T08:45:12.820Z",
    "7.3.0-beta.16c6462": "2023-12-03T09:08:12.787Z",
    "7.3.0-beta.6e32b20": "2023-12-04T22:01:52.885Z",
    "7.3.0-beta.01256d2": "2023-12-04T23:03:16.905Z",
    "7.3.0-beta.ca3f7f5": "2023-12-09T18:40:15.574Z",
    "7.3.0-beta.6062919": "2023-12-09T21:13:00.338Z",
    "7.3.0-beta.4df0638": "2023-12-10T20:59:22.104Z",
    "7.3.0-beta.d293309": "2023-12-14T10:19:15.260Z",
    "7.3.0-beta.38bb1d7": "2023-12-14T21:21:59.192Z",
    "7.3.0-beta.e0b37b9": "2023-12-14T21:34:22.111Z",
    "7.3.0-beta.1c23854": "2023-12-15T15:54:43.909Z",
    "7.3.0-beta.66401a3": "2023-12-17T22:32:15.416Z",
    "7.3.0-beta.9479ed4": "2023-12-24T12:19:43.081Z",
    "7.3.0": "2023-12-29T09:58:45.591Z",

we may find actually two releases done on 9.12.2023 and it was the first one (so not the one I suspected). Actually it turned out it was connected to dependency update which was kinda done in between our releases https://github.com/pnp/cli-microsoft365/commit/ca3f7f52c851062d058a7cc93b8822925b23873f#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519

it turned out that the thing that caused this issue is ora, a library we use to present spinner in terminal. In this change we update it from version 5.4.1 to 7.0.1 which was latest at that time. when I checked in Azure Cloud Shell I downloaded and rebuild CLI in Azure Cloud Shell and run the local version and no response (so reproduced this error in latest CLI version we have) image

Then when I updated ora to 5.4.1 image

and I rebuild it started working properly image also the version and status and login and other commands as well (and the loader as well) image

so it seems reverting ora to older version 5.4.1 works properly. when checking it even more I found this issue in ora GitHub repo, seems kinda related. https://github.com/sindresorhus/ora/issues/229

also when checking on npm version the 5.4.1 seems to be most popular version of this package, maybe for a good reason image

Adam-it commented 3 months ago

@pnp/cli-for-microsoft-365-maintainers what I propose is to just downgrade the ora package to older version 5.4.1 which solves this issue. What do you think? Otherwise, I have no idea (at least now) why this happens and it seems strictly related to our dependency not to our code.

waldekmastykarz commented 3 months ago

Awesome find! I agree that downgrading is a good option. One more thing I'd like to suggest is that we consider removing it altogether. If such a considerably small thing causing so much trouble, I wonder if it's worth it and if it truly makes the UX so much better. Thoughts?

martinlingstuyl commented 3 months ago

Hi guys, I agree it's a good solution, and I may even agree with @waldekmastykarz on removing it altogether. It's a gimmick after all. I also sometimes run into these little annoyances where it is part of the command output in scripting mode. I built that myself as part of an update to how it works in PowerShell with output streams, but I'm not convinced of its usefulness these days...

Adam-it commented 3 months ago

@waldekmastykarz, @martinlingstuyl let's not forget the spinner is an additional problem in GH actions or scripts running in pipelines. If we do not disable them they are logged and part of output on this flow. In CLI GH actions we have an issue for that. If someone uses CLI in ADO he or she needs to remember to do that (disable spinner) or just don't care (which is what we scaffold in the pipeline step). It's also an additional thing I need to handle when implementing dedicated YAML tasks for ADO for CLI which is in progress. We may remove the spinner for v8?

Adam-it commented 3 months ago

Ok thanks @waldekmastykarz and @martinlingstuyl for clarifying. I will open a PR with a downgrade of this package to fix the issue shortly. I will include detail info how to test it CLI in Azure Cloud Shell from local files/build as it is a bit hacky. I will also open a separate issue for v8 to drop the spinner pointing out all the problems we have with it

waldekmastykarz commented 3 months ago

If we keep the existing configuration options intact, then I wouldn't consider removing the spinner a breaking change, as it's a UI feature. In v8, we could then drop the then no longer necessary configuration options, which would constitute a breaking change. I suggest we avoid temporary workarounds and remove the package already now.

Adam-it commented 3 months ago

@waldekmastykarz but won't this seem like a big if we remove the spinner dependency already and have some configs in CLI settings that when set won't bring any effect? Of course we may drop them as deprecated but still it may seem kinda strange. Also some may have already m365 setup --scripting which should hide loader which will bring no effect. Won't it be strange?

Adam-it commented 3 months ago

so to sum up we may do the following to fix this issue:

  1. simply lower down the ora package to 5.4.1 and for v8 remove ora and all of spinner related configs
  2. remove ora package and spinner in code (so it will stop showing from next minor release) and remove spinner related configs in v8
  3. ????

@pnp/cli-for-microsoft-365-maintainers what should we do 👆 I vote for option 1. 🤷‍♂️ but I don't have a hard opinion about it

waldekmastykarz commented 3 months ago

@waldekmastykarz but won't this seem like a big if we remove the spinner dependency already and have some configs in CLI settings that when set won't bring any effect? Of course we may drop them as deprecated but still it may seem kinda strange. Also some may have already m365 setup --scripting which should hide loader which will bring no effect. Won't it be strange?

I think we overestimate the amount of attention people pay to it. I think the spinner is a nice gimmick but not critical to the CLI functionality. Not to mention, it leads to a bunch of issues that we've addressed partly. If we know that we're going to remove it in the end, why not save us double effort and do it right away? We're going to keep the settings options for now, not to break anyone and we'll clean them up in the next major. I suggest we remove it altogether right away.

martinlingstuyl commented 3 months ago

I agree with @waldekmastykarz on this. I like spinners, but not that it causes headaches all around. Let's remove it!

Adam-it commented 3 months ago

Ok agreed. I will remove it. Will try to do it today (tonight of course 😉) For removing the settings I will create separate issue for v8. Thanks 👏👍