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

Support list_of_files and project CLI lint modes for PSScriptAnalyzer linter #1994

Open
Isalgeon opened this issue Oct 23, 2022 · 19 comments
Open
Labels
enhancement New feature or request nostale This issue or pull request is not stale, keep it open

Comments

@Isalgeon
Copy link
Contributor

Isalgeon commented Oct 23, 2022

Is your feature request related to a problem? Please describe.
I'm currently in the process of integrating/enabling the PowerShell linter (PSScriptAnalyzer) and I'm running into some serious performance hick-ups.

Describe the solution you'd like
Enable the list_of_files capability for the PSScriptAnalyzer linter by default.

Describe alternatives you've considered
I've looked at the Invoke-ScriptAnalyzer command documentation and it appears to support both single file and directory-based linting.
Providing a directory to the -Path argument along with the -Recurse argument appears to work properly in-line with expectations.

Additional context
In my repository with 117 PowerShell scripts and modules, the performance difference is off the charts:

  • Roughly 202 seconds via MegaLinter (file CLI lint mode).
  • Roughly 4 seconds via command line (list_of_files CLI lint mode simulation).

Configuring list_of_files CLI lint mode manually (i.e. POWERSHELL_POWERSHELL_CLI_LINT_MODE: "list_of_files") yields the following error:

Traceback (most recent call last):
  File "/usr/local/lib/python3.10/runpy.py", line 196, in _run_module_as_main
    return _run_code(code, main_globals, None,
  File "/usr/local/lib/python3.10/runpy.py", line 86, in _run_code
    exec(code, run_globals)
  File "/megalinter/run.py", line 9, in <module>
    linter = megalinter.Megalinter({"cli": True})
  File "/megalinter/MegaLinter.py", line 121, in __init__
    self.load_linters()
  File "/megalinter/MegaLinter.py", line 475, in load_linters
    all_linters = linter_factory.list_all_linters(linter_init_params)
  File "/megalinter/linter_factory.py", line 30, in list_all_linters
    descriptor_linters = build_descriptor_linters(
  File "/megalinter/linter_factory.py", line 117, in build_descriptor_linters
    linter_instance = linter_class(linter_init_params, instance_attributes)
  File "/megalinter/linters/PowershellLinter.py", line 13, in __init__
    super(PowershellLinter, self).__init__(params, linter_config)
  File "/megalinter/Linter.py", line 283, in __init__
    self.load_config_vars(params)
  File "/megalinter/Linter.py", line 576, in load_config_vars
    raise KeyError(
KeyError: 'You can not override POWERSHELL_POWERSHELL cli_lint_mode with list_of_files, as it can process files only one by one. If you think it could be done, post an issue :)'
@Isalgeon Isalgeon added the enhancement New feature or request label Oct 23, 2022
@nvuillam
Copy link
Member

@Isalgeon good catch :)

Please could you provide the working command line examples of your tests ?
We have to rebuild them in custom Linter class https://github.com/oxsecurity/megalinter/blob/main/megalinter/linters/PowershellLinter.py

@Isalgeon
Copy link
Contributor Author

The command that I used during testing was Invoke-ScriptAnalyzer -Path "$(pwd)" -Recurse.

I noticed you added an improvement a while back to apply fixes #93. Although it's technically outside the scope of this issue, I can see that the complete out-of-the-box command can be Invoke-ScriptAnalyzer -EnableExit -Fix -Path "$(pwd)" -Recurse -Settings ".powershell-psscriptanalyzer.psd1".

The above command doesn't account for all the various configuration settings though. ;-)

@Isalgeon
Copy link
Contributor Author

Reiterating over my above suggestion, I realized that it's more in line with the project CLI lint mode than list_of_files. Configuring the project CLI lint mode actually leads to following error though:

❌ Linted [POWERSHELL] files with [powershell]: Found 1 error(s) - (1.64s)
- Using [powershell v7.2.6] https://oxsecurity.github.io/megalinter/latest/descriptors/powershell_powershell
- MegaLinter key: [POWERSHELL_POWERSHELL]
- Rules config: [/.powershell-psscriptanalyzer.psd1]
--Error detail:
Invoke-ScriptAnalyzer: Cannot find path '/tmp/lint/None' because it does not exist.

Luckily for us, it's possible to pipe an array of paths to the Invoke-ScriptAnalyzer command that should work for MegaLinter.
For example:

@("/tmp/lint/Test-Script.ps1", "/tmp/lint/modules/Some-Module.psm1") | Invoke-ScriptAnalyzer -EnableExit -Fix -Settings ".powershell-psscriptanalyzer.psd1"

@nvuillam
Copy link
Member

my knownledge of Powershell scripting is close to zero.... maybe you'd like to make the pull request ? :)

@Isalgeon
Copy link
Contributor Author

The same goes for my Python knowledge but I want to learn it anyway so now I have a concrete and worthy goal. :)
I'll give it a go probably later this week.

@nvuillam
Copy link
Member

Thanks :)
Basically, the following method must be able to build a correct command line for cli_lint_mode file, list_of_files and project :)

def build_lint_command(self, file=None):

@Isalgeon Isalgeon changed the title Enable list_of_files capability for PSScriptAnalyzer linter Support list_of_files and project CLI lint modes for PSScriptAnalyzer linter Oct 24, 2022
@Isalgeon
Copy link
Contributor Author

@nvuillam I'll scope my changes to only the 2 lint modes for the PSScriptAnalyzer. I'll introduce the automatic fixes in a separate follow-up PR and tie it to your original issue #93.

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Nov 24, 2022
@Isalgeon
Copy link
Contributor Author

Isalgeon commented Dec 7, 2022

This issue is not stale. I haven't had much time to work on this yet, but I have secured a few hours at work to get a solution underway.

@github-actions github-actions bot removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Dec 8, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jan 8, 2023

This issue has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Jan 8, 2023
@nvuillam nvuillam removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Jan 8, 2023
@Kurt-von-Laven Kurt-von-Laven added the nostale This issue or pull request is not stale, keep it open label Jan 10, 2023
@Isalgeon
Copy link
Contributor Author

@nvuillam I've yet to confirm it to be 100% sure, but at first glance, it appears #2176 already tackled this issue in a 'different' linter. Oddly enough, both the PowerShell and PowerShell_formatter linters appear to use the same underlying linter (PSScriptAnalyzer).

I'll make sure to confirm this tomorrow at the latest on the PR level and also make sure to test-drive the latest Docker image.

I'm super excited about versions 6.17.0 and 6.18.0! 🤩

@nvuillam
Copy link
Member

@Isalgeon indeed, the great @bdovaz allowed -Fix parameter to be sent :)

But it does not solve the case of not called the linter file by file or with a -Recurse, and additional PR may be necessary

You may test with MegaLinter beta version, not sure all updates has already been released :)

@nvuillam
Copy link
Member

nvuillam commented Jan 10, 2023

Basically, I don't think list_of_files could work (except if you find so in the doc), but we probably can update PowershellLinter.py to add -Path "{self.workspace}" -Recurse if POWERSHELL_POWERSHELL_CLI_LINT_MODE: project is defined :)

@nvuillam
Copy link
Member

@bdovaz maybe such update could be added in #2239 ? 😄

@Isalgeon
Copy link
Contributor Author

Isalgeon commented Jan 10, 2023

I don't think there's an out-of-the-box way of getting the list_of_files to work other than the suggestion I provided earlier in this issue thread. A CLI equivalent of the earlier suggestion should also work for us; I will have to verify this.

I would already be fine with it being on a project basis since I lint everything anyway. ;-)

@bdovaz
Copy link
Collaborator

bdovaz commented Jan 10, 2023

@bdovaz maybe such update could be added in #2239 ? 😄

@nvuillam done, I think fa175b3

@Isalgeon
Copy link
Contributor Author

Isalgeon commented Jun 6, 2023

@nvuillam, I've finally had the time to check this action off my list and yours. :-)

I tested both PowerShell linters and the original still suits my project the best.
The project CLI lint mode also improves performance by ~44 seconds (104 seconds down from 148) for 140 PowerShell files.

I want to close this issue as I don't see the need for the list_of_files lint mode for my projects and configuration.

However, I did notice that the performance of the linter does not quite match what I expected based on invoking the Invoke-ScriptAnalyzer cmdlet directly. If I invoke the command that MegaLinter builds (from what I can gather), it finishes in ~5 seconds on the same set of files. The cmdlet that I used is: Invoke-ScriptAnalyzer -EnableExit -Settings ".powershell-psscriptanalyzer.psd1" -Path $(pwd) -Recurse -Fix

Could you clarify the difference and what I'm missing? Maybe @bdovaz has some insight here as well.

If this is beyond the scope of this issue, I'm happy to fire a fresh one. ;-)

@Isalgeon
Copy link
Contributor Author

@nvuillam @bdovaz, can either of you shed some light on the observation above? :-)

@nvuillam
Copy link
Member

@Isalgeon i'm still a newbie with dotnet stuff :/
Would you like to make a pull requests :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request nostale This issue or pull request is not stale, keep it open
Projects
None yet
Development

No branches or pull requests

4 participants