Fallback to syntax-check if ansible-lint is not installed or disabled #5

priyamsahoo · 2021-08-23T08:56:29Z

Add logic to fallback to ansible syntax-check for diagnostics in-case ansible-lint is not installed (or disabled) in the user's environment.

Fixes: ansible/vscode-ansible#146

priyamsahoo · 2021-08-23T09:01:00Z

@ganeshrn, I haven't created a separate ansiblePlaybook class. The reason is because, I didn't find any method to go into that class which can further be inherited by ansibleSyntaxCheck class. I find all the methods to be unique to ansibleSyntaxCheck class itself.

Need your review and thoughts before implementing the ansiblePlaybook class.

ganeshrn · 2021-08-23T09:18:09Z

/cc @tomaciazek

tomaciazek

Good work, but there are some issues and copy-paste left-overs.

The libraryChecker name sounds misleading to me, given what it does and what it works with, but maybe that's just me.

src/interfaces/extensionSettings.ts

src/providers/validationProvider.ts

src/services/ansibleSyntaxCheck.ts

src/utils/libraryChecker.ts

src/services/ansibleSyntaxCheck.ts

src/utils/libraryChecker.ts

src/services/ansibleSyntaxChecker.ts

src/utils/libraryChecker.ts

src/providers/validationProvider.ts

src/services/ansibleLint.ts

src/services/ansiblePlaybook.ts

src/providers/validationProvider.ts

src/services/ansibleLint.ts

tomaciazek

Is use of console.debug on purpose? If not, all instances should be converted to use of connection.console.

src/services/ansibleLint.ts

src/services/ansiblePlaybook.ts

src/services/workspaceManager.ts

webknjaz · 2021-09-30T17:11:36Z

Is use of console.debug on purpose? If not, all instances should be converted to use of connection.console.

I think debug is for devs (shows up in the JS console) and console as for end-users.

tomaciazek · 2021-09-30T18:26:15Z

Is use of console.debug on purpose? If not, all instances should be converted to use of connection.console.

I think debug is for devs (shows up in the JS console) and console as for end-users.

Alright, if that is a conscious use, then I'm okay with that. If this is something that should be visible in the Ansible Server logs (available for the end-user), then it should be changed.

priyamsahoo · 2021-09-30T18:32:07Z

Is use of console.debug on purpose? If not, all instances should be converted to use of connection.console.

That's for the devs. These messages appear in the log and can be useful for debugging purposes. I did not intend those messages for the users. That's why I do not want to use connection.console

for more information, see https://pre-commit.ci

…e-server into syntax-check

ssbarnea · 2021-10-06T06:47:22Z

the linter can easily disappear (python version upgrade) and we need to inform the user that he is running in without it.

Think about divergent behaviour and bug reports from users not having the linter and expecting it to run. There will be LOTs.

I am not saying that we should necessarily display a popup error message but at least we need to make it easy to discover the source of the problem. Two thinks come into my mind:

can we add a collapsed notification? i remember the bell from status bug were vscode updates appear, is not intrusive but it is a place were people would look when something does not work as excepted.
tooltip/hover on Ansible language status bar to display environment info like version of ansible and ansible lint.

Ideally we should have prompted the user to install the linter automatically like vscode does for flake8 for example. Sadly that functionality is hard to implement. Microsoft did not make easy to reuse the install missing python dependency.

webknjaz · 2021-10-06T23:09:59Z

@ssbarnea your suggestion sounds reasonable, however, I'd move the implementation/discussion out of this PR. I'm sure Priyam can't wait to get this merged after so many rounds of review.

This change is done to support notification generation from the validation provider so that we can generate a notification for falling back to syntax-check from validation provider and not from the ansible lint service.

…tionProvider

priyamsahoo · 2021-10-07T09:48:11Z

@ssbarnea, @webknjaz, thanks for the suggestions. But as @webknjaz said, we can move this discussion out of the PR :)

src/services/ansibleLint.ts

webknjaz · 2021-10-08T09:07:17Z

src/services/ansiblePlaybook.ts

+export class AnsiblePlaybook {
+  private useProgressTracker = false;
+
+  /**


I think this is missing some text description.

priyamsahoo requested a review from ganeshrn August 23, 2021 08:56

ganeshrn requested a review from tomaciazek August 23, 2021 09:18

tomaciazek suggested changes Aug 23, 2021

View reviewed changes

priyamsahoo requested a review from tomaciazek August 31, 2021 06:50

ganeshrn reviewed Aug 31, 2021

View reviewed changes

src/utils/libraryChecker.ts Outdated Show resolved Hide resolved

ganeshrn reviewed Aug 31, 2021

View reviewed changes

src/services/ansibleSyntaxChecker.ts Outdated Show resolved Hide resolved

ganeshrn reviewed Aug 31, 2021

View reviewed changes

src/utils/libraryChecker.ts Outdated Show resolved Hide resolved

priyamsahoo requested a review from ganeshrn August 31, 2021 14:34

webknjaz reviewed Sep 8, 2021

View reviewed changes

src/providers/validationProvider.ts Outdated Show resolved Hide resolved

webknjaz reviewed Sep 8, 2021

View reviewed changes

src/services/ansibleLint.ts Outdated Show resolved Hide resolved

webknjaz reviewed Sep 8, 2021

View reviewed changes

src/services/ansiblePlaybook.ts Outdated Show resolved Hide resolved