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

npm.bootstrap does not return True (clean) with test=true and no changes #48677

Closed
OrlandoArcapix opened this Issue Jul 19, 2018 · 3 comments

Comments

Projects
None yet
3 participants
@OrlandoArcapix
Contributor

OrlandoArcapix commented Jul 19, 2018

Description of Issue/Question

When using the npm.bootstrap state with test=true, it always returns a "changes will be made" result.

Setup

Sample sls:

bootstrap my npm app:
  npm.bootstrap:
    - name: /path/to/npm/app
    - user: someuser

Steps to Reproduce Issue

  1. Set up an npm application (outside of the scope of this bug report for now!)
  2. Use npm.bootstrap state set up its dependencies (or confirm after the fact)
  3. Run npm.bootstrap state with test=true

Versions Report

I've checked the code - looks like it's in the latest in master too.

Salt Version:
           Salt: 2017.7.4

Dependency Versions:
           cffi: 1.11.5
       cherrypy: Not Installed
       dateutil: 2.7.3
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.10
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: 0.21.1
           Mako: 1.0.7
   msgpack-pure: Not Installed
 msgpack-python: 0.5.1
   mysql-python: 1.2.5
      pycparser: 2.14
       pycrypto: 2.6.1
   pycryptodome: 3.4.3
         pygit2: Not Installed
         Python: 2.7.5 (default, Aug  4 2017, 00:39:18)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 15.3.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.1.4

System Versions:
           dist: centos 7.4.1708 Core
         locale: UTF-8
        machine: x86_64
        release: 3.10.0-693.17.1.el7.x86_64
         system: Linux
        version: CentOS Linux 7.4.1708 Core
@OrlandoArcapix

This comment has been minimized.

Contributor

OrlandoArcapix commented Jul 19, 2018

I have a patch for this - will get a pull request in shortly:

diff --git a/salt/states/npm.py b/salt/states/npm.py
index 4c58d3ae03..87ed0fe9ff 100644
--- a/salt/states/npm.py
+++ b/salt/states/npm.py
@@ -275,9 +275,13 @@ def bootstrap(name, user=None, silent=True):
     if __opts__['test']:
         try:
             call = __salt__['npm.install'](dir=name, runas=user, pkg=None, silent=silent, dry_run=True)
-            ret['result'] = None
-            ret['changes'] = {'old': [], 'new': call}
-            ret['comment'] = '{0} is set to be bootstrapped'.format(name)
+            if call:
+                ret['result'] = None
+                ret['changes'] = {'old': [], 'new': call}
+                ret['comment'] = '{0} is set to be bootstrapped'.format(name)
+            else:
+                ret['result'] = True
+                ret['comment'] = '{0} is already bootstrapped'.format(name)
         except (CommandNotFoundError, CommandExecutionError) as err:
             ret['result'] = False
             ret['comment'] = 'Error Bootstrapping \'{0}\': {1}'.format(name, err)

OrlandoArcapix added a commit to OrlandoArcapix/salt that referenced this issue Jul 19, 2018

@gtmanfred gtmanfred added this to the Approved milestone Jul 20, 2018

@gtmanfred

This comment has been minimized.

Contributor

gtmanfred commented Jul 20, 2018

Thanks for the PR!
Daniel

rallytime added a commit that referenced this issue Jul 20, 2018

Merge pull request #48678 from OrlandoArcapix/fix-npm-dryrun-test
Fix for issue #48677 - return clean npm.bootstrap on no changes
@rallytime

This comment has been minimized.

Contributor

rallytime commented Jul 20, 2018

Closed via #48678

@rallytime rallytime closed this Jul 20, 2018

cro added a commit to cro/salt that referenced this issue Jul 26, 2018

Fix for issue saltstack#48677 - return True when no changes are to be…
… made with npm.bootstrap with test=true
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment