Salt: git.latest with unless command fails after upgrade to 2018.3.3

[ata-sql]

Description of Issue/Question

after upgrading (Centos 7) saltstack to 2018.3.3 git.latest state with unless "git status" starts to fail.

Setup

salt-repo-git:
  git.latest:
    - name: ssh://git@{{ git_repo_url }}/sal/salt-apache-vhosts-test.git
    - target: {{ git_repo_path }}
    - user: apache
    - unless:
      - cd {{ git_repo_path }}
      - git status >/dev/null 2>&1

Steps to Reproduce Issue

----------
          ID: salt-repo-git
    Function: git.latest
        Name: ssh://[email protected]:7999/sal/salt-apache-vhosts-test.git
      Result: False
     Comment: An exception occurred in this state: Traceback (most recent call last):
                File "/usr/lib/python2.7/site-packages/salt/state.py", line 1913, in call
                  **cdata['kwargs'])
                File "/usr/lib/python2.7/site-packages/salt/loader.py", line 1898, in wrapper
                  return f(*args, **kwargs)
                File "/usr/lib/python2.7/site-packages/salt/states/git.py", line 707, in latest
                  run_check_cmd_kwargs, onlyif, unless
                File "/usr/lib/python2.7/site-packages/salt/states/git.py", line 3370, in mod_run_check
                  if __salt__['cmd.retcode'](unless, **cmd_kwargs) == 0:
                File "/usr/lib/python2.7/site-packages/salt/modules/cmdmod.py", line 2088, in retcode
                  **kwargs)
                File "/usr/lib/python2.7/site-packages/salt/modules/cmdmod.py", line 603, in _run
                  proc = salt.utils.timed_subprocess.TimedProc(cmd, **kwargs)
                File "/usr/lib/python2.7/site-packages/salt/utils/timed_subprocess.py", line 44, in __init__
                  args = salt.utils.stringutils.to_bytes(args)
                File "/usr/lib/python2.7/site-packages/salt/utils/stringutils.py", line 63, in to_bytes
                  return to_str(s, encoding, errors)
                File "/usr/lib/python2.7/site-packages/salt/utils/stringutils.py", line 118, in to_str
                  raise TypeError('expected str, bytearray, or unicode')
              TypeError: expected str, bytearray, or unicode
     Started: 05:36:17.256304
    Duration: 54.61 ms
     Changes:   

when unless section is removed, state works fine

Versions Report

Salt Version:
           Salt: 2018.3.3

Dependency Versions:
           cffi: Not Installed
       cherrypy: Not Installed
       dateutil: Not Installed
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.7.2
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.5.6
   mysql-python: Not Installed
      pycparser: Not Installed
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.5 (default, Jul 13 2018, 13:06:57)
   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.5.1804 Core
         locale: UTF-8
        machine: x86_64
        release: 4.18.16-200.fc28.x86_64
         system: Linux
        version: CentOS Linux 7.5.1804 Core

[dwoz]

@ata-sql Thank you for reporting this.

[terminalmage]

onlyif and unless do not support a list of commands.

[terminalmage]

It looks like it does in cmd states but was implemented differently for git states. At any rate, the fact that a list is supported is not documented anywhere, which is also problematic. I'll try to get this normalized and improve the documentation.

[terminalmage]

There's also a separate bug here, which is what is actually causing the traceback. 1f7d50d194c86128ec126b6c619e0a885f4ea250 broke this by assuming that the command is always a string. A command can be passed to TimedProc as either a string or a list of args, just like subprocess.Popen.

[ata-sql]

Well, I didn't know it wasn't supported. I found it somwhere on the internet (list of commands) and it worked. Thanks for taking analyzing it.

[terminalmage]

@ata-sql yeah, it should work if you use the following:

    - unless: cd {{ git_repo_path }} && git status

[ata-sql]

It looks like a hack, array of commands looks much nicer.

[terminalmage]

Well I still plan on normalizing it so that a list of commands works. But, if you don't want to change it then I guess you'll have to live with it not working until we get a new release out.

[terminalmage]

For that matter, you could also use git -C {{ git_repo_path }} status and avoid an unnecessary cd command.

[terminalmage]

OK, I've opened https://github.com/saltstack/salt/pull/50456 to support a list of commands for unless/onlyif. Apparently, the default handling of unless supports a list, but when overridden in a state module (as done in the cmd state, as well as a few others), it's up to the implementation of that state module's mod_run_check function, which for git states did not support a list.

@ata-sql However, you should know that the commands are executed in separate calls to cmd.retcode, so you will not be able to use your example with the cd and git status commands separate from one another. You will need to use my example with &&, or use -C to specify the gitdir. And for older git releases, such as the one on CentOS 7, -C is not available.

[garethgreenaway]

Closing this out, if the problem persists please comment & we'll re-open the issue or feel free to open a new issue.