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

git.latest with unless command fails after upgrade to 2018.3.3 #50218

Closed
ata-sql opened this issue Oct 25, 2018 · 11 comments

Comments

Projects
None yet
6 participants
@ata-sql
Copy link

commented Oct 25, 2018

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://git@bitbucket-1.efinity.local: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

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2018

@ata-sql Thank you for reporting this.

@terminalmage

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

onlyif and unless do not support a list of commands.

@terminalmage

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

There's also a separate bug here, which is what is actually causing the traceback. 1f7d50d 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

This comment has been minimized.

Copy link
Author

commented Nov 8, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

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

    - unless: cd {{ git_repo_path }} && git status
@ata-sql

This comment has been minimized.

Copy link
Author

commented Nov 8, 2018

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

@terminalmage

This comment has been minimized.

Copy link
Contributor

commented Nov 8, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2018

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

@terminalmage

This comment has been minimized.

Copy link
Contributor

commented Nov 9, 2018

OK, I've opened #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

This comment has been minimized.

Copy link
Member

commented Jan 9, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.