ansible / vscode-ansible

vscode/vscodium extension for providing Ansible auto-completion and integrating quality assurance tools like ansible-lint, ansible syntax check, yamllint, molecule and ansible-test.
https://ansible.readthedocs.io/projects/vscode-ansible/
MIT License
337 stars 77 forks source link

Gracefully handle playbook generation and explanation not available #1366

Closed manstis closed 2 weeks ago

manstis commented 3 weeks ago

This PR moves the generic handling of error code's from the backend service in to the Language Server.

Exceptions thrown by the Language Server are handled by JSON-RPC and converted into messages following a standard format.

We, however, want to preserve the code from the backend service and show meaningful messages to users.

The response from the Language Server is now always considered to be successful to avoid JSON-RPC handling exceptions but the payload represents either success or failure state. This is comparable to how shell scripting, C etc and Go handle exceptions: The function calls always succeed however the result may indicate success (rc=0) or failure (rc>0) states.

manstis commented 2 weeks ago

Hi @ssbarnea @TamiTakamiya @goneri

I wonder if you can advise... when running

task build
task package
task test

The changes I've made moving a utility function from the extension to LSP are not found and the extension shows an error message:

Activating extension 'redhat.ansible' failed: Cannot find module '@ansible/ansible-language-server/src/utils/handleApiError' Require stack:

  • /home/manstis/workspaces/git/manstis/forks/vscode-ansible/out/client/src/features/lightspeed/api.js
  • /home/manstis/workspaces/git/manstis/forks/vscode-ansible/out/client/src/features/lightspeed/base.js
  • /home/manstis/workspaces/git/manstis/forks/vscode-ansible/out/client/src/extension.js
  • /home/manstis/workspaces/git/manstis/forks/vscode-ansible/out/test-resources/VSCode-linux-x64/resources/app/out/vs/loader.js
  • /home/manstis/workspaces/git/manstis/forks/vscode-ansible/out/test-resources/VSCode-linux-x64/resources/app/out/bootstrap-amd.js
  • /home/manstis/workspaces/git/manstis/forks/vscode-ansible/out/test-resources/VSCode-linux-x64/resources/app/out/bootstrap-fork.js.

However if I install the .vsix built everything runs fine.

Is there some magic I need to perform (especially for this PR?)

Test pass if I run:-

yarn als-compile
yarn compile
yarn webpack-dev
yarn package
yarn coverage-ui-with-mock-lightspeed-server
TamiTakamiya commented 2 weeks ago

I will run test on my machine. I guess the failure occurred in "E2E" test, i.e. even though

yarn als-compile
yarn compile
yarn webpack-dev
yarn package
yarn coverage-ui-with-mock-lightspeed-server

passes,

yarn als-compile
yarn compile
yarn webpack-dev
yarn package
yarn coverage-all

likely fails. Let me verify it on my machine.

TamiTakamiya commented 2 weeks ago

Yes, yarn coverage-all failed after yarn package

   :
Error: Cannot find module '@ansible/ansible-language-server/src/utils/handleApiError'
Require stack:
- /home/ttakamiy/git/ansible/vscode-ansible/out/client/src/features/lightspeed/api.js
- /home/ttakamiy/git/ansible/vscode-ansible/out/client/src/features/lightspeed/base.js
- /home/ttakamiy/git/ansible/vscode-ansible/out/client/src/extension.js
  :

ansible-language-server (ALS) runs in a separate process (previously it was in a separate repository). When we package it in a .vsix file, we stores ALS files under out/server and configure the server uses the files in that directory. So originally it was not designed to share files in ALS in VS Code extension.

When we package .vsix file, we run webpack and it resolves dependencies and creates bundled js files. On client (VS Code extension) part, it is provided as extension.js file. I believe that bundled extension.js contains the handleApiError.js. So it works with .vsix.

To resolve this isssue, I think we can create a js file, which is webpacked even when E2E test is running like what I did for "SyntaxHighlighter", which is used for Playbook Generation. I will describe it later (after standup).

TamiTakamiya commented 2 weeks ago

To resolve this isssue, I think we can create a js file, which is webpacked even when E2E test is running like what I did for "SyntaxHighlighter", which is used for Playbook Generation. I will describe it later (after standup).

In webpack.config.ts, I added a new entry:

  syntaxHighlighter: "./src/features/utils/syntaxHighlighter.ts",

We define output location as

"[name]/src/[name].js"

so yarn webpack or yarn webpack-dev will generate the bundled js as out/syntaxHighlighter/src/syntaxHighlighter.js

src/features/lightspeed/playbookGeneration.ts has the following ugly code to load syntaxHighlighter.js:

        // eslint-disable-next-line @typescript-eslint/no-explicit-any
        let syntaxHighlighter: any;
        try {
          syntaxHighlighter =
            await require(/* webpackIgnore: true */ "../../syntaxHighlighter/src/syntaxHighlighter");
        } catch (error) {
          syntaxHighlighter =
            await require(/* webpackIgnore: true */ "../../../../syntaxHighlighter/src/syntaxHighlighter");
        }

In the production env, the dynamic loading occurs from (bundled) src/extension.js. That's why I need to have two dynamic import calls...

So if we do

  1. Add an entry to webpack.config.ts
  2. Statically link the entry to handleApiError.js
  3. Dynamically link the webpacked js where it is needed

it should work... but it may be too much for this...

For this case, simply creating a symlink might work. Let me try that approach.

TamiTakamiya commented 2 weeks ago

@manstis Still trying to find a better solution... For running E2E test, we are using "testRunner", which is descrbed here. If the testRunner could resolve the dependencies, we do not have to change the runtime code because .vsix runs w/o issues. I think there should be a way to specify additional path and checking the doc.

TamiTakamiya commented 2 weeks ago

@mantis THIS is my atttempt to fix this with a webpacked js. Since it's too ugly, I will spend some more time on searching better solutions after fixing the Playbook Explanation issues with task files/role-based playbooks.

TamiTakamiya commented 2 weeks ago

@manstis I have simplified my test fix. Would you take a look?

goneri commented 2 weeks ago

@manstis thank you Michael!