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

Adding powershell linter hook - best practice #2645

Open
juju4 opened this issue Dec 17, 2022 · 10 comments
Open

Adding powershell linter hook - best practice #2645

juju4 opened this issue Dec 17, 2022 · 10 comments

Comments

@juju4
Copy link

juju4 commented Dec 17, 2022

search you tried in the issue tracker

powershell, return code, exit code

describe your issue

I'm working on a repository containing powershell scripts.
I have a pre-commit already enabled for other checks and I want to add powershell linting with https://github.com/PowerShell/PSScriptAnalyzer.

I added a hook config as
PowerShell/PSScriptAnalyzer@master...juju4:PSScriptAnalyzer:devel-precommit

-   id: powershell_scriptanalyzer
    name: Powershell lint with PSScriptAnalyzer
    description: This runs PSScriptAnalyzer on your powershell files
    entry: pre-commit powershell_scriptanalyzer
    language: script
    files: ^.*\.ps1$
    types: [text]
    entry: pwsh -Command "Invoke-ScriptAnalyzer -Settings PSGallery -Recurse -ReportSummary -EnableExit -Path ."
    require_serial: false
    additional_dependencies: []
    minimum_pre_commit_version: '0'
    pass_filenames: false
    verbose: true

In the target repo, I added following config

-   repo: https://github.com/juju4/PSScriptAnalyzer
    rev: 4d6a4a36c78df215bc8fa637d05929eb7a9112d9
    hooks:
    -   id: powershell_scriptanalyzer
        entry: /usr/local/bin/pwsh -Command "Invoke-ScriptAnalyzer -Settings PSGallery -Recurse -ReportSummary -EnableExit -Path ."

3 minor issues as using entry to deal with them for now.

  • As I'm executing on non-windows system, I have to customize/duplicate entry value with pwsh full path. not sure if better way?
  • to support powershell cmdlet include/exclude rules or severity (https://github.com/PowerShell/PSScriptAnalyzer/blob/master/docs/Cmdlets/Invoke-ScriptAnalyzer.md), I again have to customize entry. any way to define pre-commit fields to pass that inside the pwsh Command? default args apply outside.
  • same problem applies if want pass_filenames=true. /usr/local/bin/pwsh -Command "Invoke-ScriptAnalyzer -Settings PSGallery -Recurse -ReportSummary -EnableExit -Path $@" did not work in my tests.

Else happy to take more comments

Thanks a lot for your work!

pre-commit --version

2.20.0

.pre-commit-config.yaml

repos:
-   repo: https://github.com/juju4/PSScriptAnalyzer
    rev: 4d6a4a36c78df215bc8fa637d05929eb7a9112d9
    hooks:
    -   id: powershell_scriptanalyzer
        entry: /usr/local/bin/pwsh -Command "Invoke-ScriptAnalyzer -Settings PSGallery -Recurse -ReportSummary -EnableExit -Path ."

~/.cache/pre-commit/pre-commit.log (if present)

No response

@asottile
Copy link
Member

you shouldn't need a full path to pwsh -- it will look up on path automatically

pass_filenames: true is the recommended way to set up hooks -- otherwise you don't really benefit from pre-commit's file matching at all and then have to re-implement it using the tool.

I think what you actually want is to use language: dotnet which will build the executables which you'll call directly without using pwsh at all

@juju4
Copy link
Author

juju4 commented Dec 17, 2022

If I don't pass the full path for pwsh, I get

- hook id: powershell_scriptanalyzer
- duration: 0s
- exit code: 1

Executable `<HOME>/.cache/pre-commit/<RANDOM>/pwsh` not found

ok for pass_filenames true but how?
if using $@ and pass_filenames true, I get

- hook id: powershell_scriptanalyzer
- duration: 0.69s
- exit code: 1

Invoke-ScriptAnalyzer: A positional parameter cannot be found that accepts argument '<MODIFIED FILES>'

on dotnet, not sure how, https://github.com/PowerShell/PSScriptAnalyzer is not a dotnet app.
from quick check, dotnet fails saying missing application pack.

@asottile
Copy link
Member

that's because you've incorrectly set language: script which is to be used for scripts

@juju4
Copy link
Author

juju4 commented Dec 18, 2022

I tested locally with following pre-commit-config

repos:
-   repo: https://github.com/juju4/PSScriptAnalyzer
    rev: 4d6a4a36c78df215bc8fa637d05929eb7a9112d9
    hooks:
    -   id: powershell_scriptanalyzer
        entry: /usr/local/bin/pwsh -Command "Invoke-ScriptAnalyzer -Settings PSGallery -Recurse -ReportSummary -EnableExit -Path ."
        language: dotnet

which should be valid unless language can't be overriden but behavior below shown it applies

full output


% pre-commit run -a powershell_scriptanalyzer
[INFO] Installing environment for https://github.com/juju4/PSScriptAnalyzer.
[INFO] Once installed this environment will be reused.
[INFO] This may take a few minutes...
An unexpected error has occurred: CalledProcessError: command: ('/bin/bash', '/usr/local/bin/dotnet', 'pack', '--configuration', 'Release', '--output', 'pre-commit-build')
return code: 145
expected return code: 0
stdout:
    7.0.100 [/usr/local/Cellar/dotnet/7.0.100/libexec/sdk]

stderr:
    The command could not be loaded, possibly because:
      * You intended to execute a .NET application:
          The application 'pack' does not exist.
      * You intended to execute a .NET SDK command:
          A compatible .NET SDK was not found.

    Requested SDK version: 3.1.424
    global.json file: /Users/USERNAME/.cache/pre-commit/repo8rpcf_st/global.json

    Installed SDKs:

    Install the [3.1.424] .NET SDK or update [/Users/USERNAME/.cache/pre-commit/repo8rpcf_st/global.json] to match an installed SDK.

    Learn about SDK resolution:
    https://aka.ms/dotnet/sdk-not-found

@asottile
Copy link
Member

yeah that output would say it's not a dotnet tool so maybe language: dotnet isn't going to work for it -- is there a way to install powershell commandlets in a specific directory? if so then potentially a language: powershell could be added to pre-commit for direct language support

@juju4
Copy link
Author

juju4 commented Dec 22, 2022

As requirement for above to work (local or remote repo), I needed pwsh to be present and do Install-Module -Name PSScriptAnalyzer as per https://learn.microsoft.com/en-us/powershell/utility-modules/psscriptanalyzer/overview?view=ps-modules#installing-psscriptanalyzer
After that, you don't need any path to call it from powershell.
I don't think that there is an option to directly call a powershell cmdlet from a non-powershell shell.
Or maybe through a wrapper script

"&" syntax may be possible from https://stackoverflow.com/questions/932291/calling-powershell-cmdlets-from-windows-batch-file
https://social.technet.microsoft.com/Forums/exchange/en-US/25e23957-d81b-468b-a683-90b6b84ed16b/calling-powershell-cmdlets-from-cmd-prompt
https://community.spiceworks.com/topic/1951541-running-a-powershell-function-from-command-line

If want to install cmdlet in a dedicated pre-commit path:
Install-Module only has a scope option (CurrentUser or AllUsers) - https://learn.microsoft.com/en-us/powershell/module/powershellget/install-module?view=powershell-7.3
It may be possible to use PSModulePath environment variable to alter install path - https://learn.microsoft.com/en-us/powershell/scripting/developer/module/installing-a-powershell-module?view=powershell-7.3

more testing needed.

@asottile
Copy link
Member

asottile commented Jan 1, 2023

yeah I think the way to make this work nicely with pre-commit would be to build into a path and use PSModulePath

@juju4
Copy link
Author

juju4 commented Jan 8, 2023

Sadly install part does not seem to work directly from my testing

PS > [Environment]::GetEnvironmentVariable("PSModulePath")
/home/user/.local/share/powershell/Modules:/usr/local/share/powershell/Modules:/home/user/.local/bin/Modules
PS > $p = [Environment]::GetEnvironmentVariable("PSModulePath")
PS > $p2 = "/home/user/.cache/pre-commit/psmodules:" + $p        
PS > [Environment]::SetEnvironmentVariable("PSModulePath",$p2)
PS > [Environment]::GetEnvironmentVariable("PSModulePath")     
/home/user/.cache/pre-commit/psmodules:/home/user/.local/share/powershell/Modules:/usr/local/share/powershell/Modules:/home/user/.local/bin/Modules
PS > Install-Module -Name PSScriptAnalyzer -Repository PSGallery -Verbose -Force
VERBOSE: Suppressed Verbose Repository details, Name = 'PSGallery', Location = 'https://www.powershellgallery.com/api/v2'; IsTrusted = 'False'; IsRegistered = 'True'.
VERBOSE: Repository details, Name = 'PSGallery', Location = 'https://www.powershellgallery.com/api/v2'; IsTrusted = 'False'; IsRegistered = 'True'.
VERBOSE: Using the provider 'PowerShellGet' for searching packages.
VERBOSE: Using the specified source names : 'PSGallery'.
VERBOSE: Getting the provider object for the PackageManagement Provider 'NuGet'.
VERBOSE: The specified Location is 'https://www.powershellgallery.com/api/v2' and PackageManagementProvider is 'NuGet'.
VERBOSE: Searching repository 'https://www.powershellgallery.com/api/v2/FindPackagesById()?id='PSScriptAnalyzer'' for ''.
VERBOSE: Total package yield:'1' for the specified package 'PSScriptAnalyzer'.
VERBOSE: Performing the operation "Install-Module" on target "Version '1.21.0' of module 'PSScriptAnalyzer'".
VERBOSE: The installation scope is specified to be 'CurrentUser'.
VERBOSE: The specified module will be installed in '/home/user/.local/share/powershell/Modules'.
VERBOSE: The specified Location is 'NuGet' and PackageManagementProvider is 'NuGet'.
VERBOSE: Downloading module 'PSScriptAnalyzer' with version '1.21.0' from the repository 'https://www.powershellgallery.com/api/v2'.
VERBOSE: Searching repository 'https://www.powershellgallery.com/api/v2/FindPackagesById()?id='PSScriptAnalyzer'' for ''.
VERBOSE: InstallPackage' - name='PSScriptAnalyzer', version='1.21.0',destination='/tmp/997172111'                       
VERBOSE: DownloadPackage' - name='PSScriptAnalyzer', version='1.21.0',destination='/tmp/997172111/PSScriptAnalyzer.1.21.0/PSScriptAnalyzer.1.21.0.nupkg', uri='https://www.powershellgallery.com/api/v2/package/PSScriptAnalyzer/1.21.0'
VERBOSE: Downloading 'https://www.powershellgallery.com/api/v2/package/PSScriptAnalyzer/1.21.0'.                        
VERBOSE: Completed downloading 'https://www.powershellgallery.com/api/v2/package/PSScriptAnalyzer/1.21.0'.              
VERBOSE: Completed downloading 'PSScriptAnalyzer'.                                                                      
VERBOSE: InstallPackageLocal' - name='PSScriptAnalyzer', version='1.21.0',destination='/tmp/997172111'                  
VERBOSE: Validating the 'PSScriptAnalyzer' module contents under '/tmp/997172111/PSScriptAnalyzer.1.21.0' path.         
VERBOSE: Test-ModuleManifest successfully validated the module manifest file '/tmp/997172111/PSScriptAnalyzer.1.21.0'.
VERBOSE: Module 'PSScriptAnalyzer' was installed successfully to path '/home/user/.local/share/powershell/Modules/PSScriptAnalyzer/1.21.0'.

also same reported here
https://stackoverflow.com/questions/42488323/how-to-install-module-into-custom-directory

And match the absence of a prefix option for Install-Module
https://learn.microsoft.com/en-us/powershell/module/powershellget/install-module?view=powershell-7.3
https://github.com/PowerShell/PowerShellGet/issues/524

Seems the only option would be download and copy files manually
https://learn.microsoft.com/en-us/powershell/module/microsoft.powershell.core/about/about_modules?view=powershell-7.3#how-to-install-a-module
quick try that seems to work

# https://www.powershellgallery.com/packages/PSScriptAnalyzer/1.21.0
# https://www.comparitech.com/net-admin/install-powershell-modules/
Invoke-WebRequest -Uri https://psg-prod-eastus.azureedge.net/packages/psscriptanalyzer.1.21.0.nupkg -Outfile /tmp/psscriptanalyzer.zip
New-Item -Path $home/.cache/pre-commit/psmodules/PSScriptAnalyzer/psscriptanalyzer.1.21.0 -ItemType Directory
Expand-Archive -Path /tmp/psscriptanalyzer.zip -DestinationPath $home/.cache/pre-commit/psmodules/PSScriptAnalyzer/1.21.0

# usage
$p = [Environment]::GetEnvironmentVariable("PSModulePath")
$p2 = "$home/.cache/pre-commit/psmodules:" + $p
[Environment]::SetEnvironmentVariable("PSModulePath", $p2)
# confirm module present
Get-Module -ListAvailable
# use it
Invoke-ScriptAnalyzer -Settings PSGallery -Recurse -ReportSummary -EnableExit -Severity Warning -Path . -ExcludeRule PSAvoidGlobalVars, PSAvoidUsingPlainTextForPassword, PSUseShouldProcessForStateChangingFunctions

Where should this configuration go?

Also the current need for a full pwsh path is annoying, especially if running pre-commit both client and server side where path may be different.

@asottile
Copy link
Member

asottile commented Jan 8, 2023

yeah the default Install-Module won't do that, but if you build and install directly you should have full control over where the thing gets installed

@martukas
Copy link

martukas commented Apr 11, 2023

I have been trying to accomplish something similar. Took some lessons from this thread. I have an adhoc implementation in my dotfiles repo:

Basically, the wrapper/hook does the following:

  1. analyzes everything in the repo
  2. filters out any files in git sub-modules
  3. sanitizes file paths to be relative
  4. adds URL links for each violated rule
  5. prints all errors sorted by file
  6. prints severity statistics
  7. panics if at least one error was reported

This seems reasonable to expect of a linter. I understand that this may be out of scope for a generalized tool as the analyzer seems to be, but does everyone really hand-roll this much in PS when they try to use this in real life?

Anyway, I realize that what I should actually be doing is taking a list of files provided by pre-commit, but I am pretty new to PowerShell, so I focused on getting the proof of concept part of making it useful. Seems the PowerShell people have had a ticket open regarding more control over what files it includes/excludes for a while now? Maybe I'm not getting something and this is much simpler to accomplish?

I think it should be possible to do this the "correct" way, but I have a few concerns:

  • I could be processing each file provided by pre-commit with a new invocation of ScriptAnalyzer and then aggregate the results before reporting and panicking, but I suspect this could be quite slow. Scripts running sandwiches of PS, git bash and Python seem slow enough on Windows, but on Linux - even more so. Probably fine to stick to just running it on the whole repo and then selecting the relevant files?
  • I guess most pre-commit scripts install their own prereqs? In this case, this might be tricky if you are not on Windows. I run this just fine on Linux with PS installed via snapd, but getting CircleCI to do it on a Ubuntu image was enough of a PITA to make me set up the CI to just do all the checks on a windows image. Would docker be the correct approach to making this truly portable? Does one then assume docker is available on windows machines?
  • is there a reasonable way to forward some of the lower level parameters Analyzer expects (like severity threshold) from pre-commit invocation? Or do we need an additional config file?

I'm new to this. I might still tough up and try to make this a generalized hook in a new repo. Anyone inspired to collaborate?

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

No branches or pull requests

4 participants
@juju4 @asottile @martukas and others