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

Add cmd module integration tests for windows and fix space in path issue #48866

Merged
merged 5 commits into from
Aug 3, 2018

Conversation

Ch3LL
Copy link
Contributor

@Ch3LL Ch3LL commented Aug 1, 2018

What does this PR do?

Adds the cmdmod module integration tests to run on windows and fixes an issue on windows when there is a space in the path name of the cwd argument.

Given this state file:

test:
  cmd.script:
    - source: salt://test.py
    - cwd: "c:\\test 2"
    - args: 'saltine crackers biscuits=yes'

When run I see this error:

----------
          ID: test
    Function: cmd.script
      Result: False
     Comment: Command 'test' run
     Started: 12:11:28.997000
    Duration: 454.0 ms
     Changes:
              ----------
              pid:
                  38548
              retcode:
                  1
              stderr:
                  '"c:\test' is not recognized as an internal or external command,
                  operable program or batch file.
              stdout:

Summary for local
------------
Succeeded: 0 (changed=1)
Failed:    1
------------
Total states run:     1
Total run time: 454.000 ms
PS C:\testing>

If i use the powershell shell it works:

test:
  cmd.script:
    - source: salt://test.ps1
    - cwd: "c:\\test 2"
    - args: 'saltine crackers biscuits=yes'
    - shell: powershell
local:
----------
          ID: test
    Function: cmd.script
      Result: True
     Comment: Command 'test' run
     Started: 12:14:27.200000
    Duration: 954.0 ms
     Changes:
              ----------
              pid:
                  39572
              retcode:
                  0
              stderr:
              stdout:
                  hello

Summary for local
------------
Succeeded: 1 (changed=1)
Failed:    0
------------

The reason this is occurring is because it tries to escape the quotes around the command leaving hte space unescaped. So the resulting command is this:

chcp 437 > nul & ^"c:\test 2\__salt.tmp.al96jv.py^" and if run in cmd.exe:

C:\Users\Administrator>chcp 437 > nul & ^"c:\test 2\__salt.tmp.al96jv.py^"
'"c:\test' is not recognized as an internal or external command,
operable program or batch file.

Also fixes an issue when it it attempts to clean the temp file here: https://github.com/saltstack/salt/blob/v2017.7.7/salt/modules/cmdmod.py#L2141

it cannot delete the temp file because it quotes the path '"<path>"' and whne it checks to see if its a valid path it fails. So this ensures we are using the inital path created from here: https://github.com/saltstack/salt/blob/v2017.7.7/salt/modules/cmdmod.py#L2084

Also this PR changes the expected return in windows on the tests so I could use some review if those returns are expected behavior or not.

What issues does this PR fix or reference?

#44997

Tests written?

Yes - adds tests and fixes something a test found

Commits signed with GPG?

Yes

@ghost ghost self-requested a review August 1, 2018 19:09
@Ch3LL Ch3LL requested review from dwoz and twangboy August 1, 2018 19:11
@@ -166,7 +166,7 @@ def get_sam_name(username):
return '\\'.join([domain, username])


def escape_argument(arg):
def escape_argument(arg, escape=True):
'''
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add this new parameter to the docs for this function

@damon-atkins
Copy link
Contributor

damon-atkins commented Aug 2, 2018

Ping @ciiqr The original coder of windows escape.

@damon-atkins
Copy link
Contributor

Why do we need chcp 437? What is the background with this?

@ciiqr
Copy link
Contributor

ciiqr commented Aug 2, 2018

This fix seems reasonable, I doubt I ever tested with cmd set as the shell, so I'm not surprised it's wrong for that case.

@rallytime rallytime requested a review from twangboy August 2, 2018 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants