Umm...wow. The salt.states.npm module is not checking for the existence of npm correctly.

[arizvisa]

Description of Issue/Question

The salt.states.npm module doesn't work because it's checking for npm support incorrectly. Back in 2017, some guy renamed the salt.modules.list function to salt.modules.list_ which broke the npm module entirely.

The reason being is that in order to verify that npm support is available, the following check is made:

def __virtual__():
    '''
    Only load if the npm module is available in __salt__
    '''
    return 'npm' if 'npm.list' in __salt__ else False, '\'npm\' binary not found on system'

Due to this states module checking for npm.list instead of the correct npm.list_, the npm state module can't be used.

Setup

This apparently has been (not) working this way since 2017-06-04. Write up a state that uses npm.installed as you will use it to repro this issue.

Steps to Reproduce Issue

Once you apply the state, you will see that it claims it's unable to locate the npm binary. This is a misnomer because if you try and exec cmd.which for npm, it will totally find it (assuming you use my PR #51785 if you're on Windows)

Versions Report

This is in the develop branch, there's no need for versions here because you can literally see the code discrepancy. As a review look at salt.states.npm and then salt.modules.npm.

salt.states.npm:31

def __virtual__():
    '''
    Only load if the npm module is available in __salt__
    '''
    return 'npm' if 'npm.list' in __salt__ else False, '\'npm\' binary not found on system'

salt.modules.npm:237:

def list_(pkg=None, dir=None, runas=None, env=None, depth=None):
    '''
    List installed NPM packages.

    If no directory is specified, this will return the list of globally-
    installed packages.

    pkg
        Limit package listing by name

[arizvisa]

Jesus christ guys. Will write up another PR in a sec...

[arizvisa]

Closed by PR #51812.

[arizvisa]

Okay. So I was able to track this down while working on the PR. It's a windows-specific issue and it seems like a common construct. Will write further results on analysis in PR #51812.

[arizvisa]

Okay, the PR has been re-created PR #51812 is deprecated in favor of PR #51813. This properly fixes the version check on the Windows platform and has been tested.

For some reason on the Windows platform the call to just npm results in an inability to locate the command. This is likely due to either there being two scripts: npm which is non-executable (on windows) bourne-script, and npm.CMD which is the actual script that cmd.exe should run.

When testing via cmd.run from salt-call, there seems to be no issue when detecting npm. However, cmd.run appears to be using cmd.run_all instead of cmd.run like the version check. As discussed in the PR, it was not determined why cmd.run_all and cmd.run seem to act differently on Windows (mostly because I really don't have time to fix these things as I'm not an engineer and thus me doing development is hard to justify as it doesn't have a related business case).

[Ch3LL]

thanks for the PR :)

[arizvisa]

Closing as the PR was merged.