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 support for win_copy, win_file and win_template modules. #9611

Merged

Conversation

jhawkesworth
Copy link
Contributor

This PR is for the supporting action plugins and integration tests needed to support the core windows modules available from ansible/ansible-modules-core#384

Both this PR and the one referenced above would be needed to add win_copy, win_file and win_template modules.

@jhawkesworth
Copy link
Contributor Author

This PR is based on Chris Church's suggestions of 14 Oct 2014 here:

https://groups.google.com/forum/#!topic/ansible-devel/Jl5qP73CiKo

I have moved the md5 checksum-calculating logic into lib/ansible/module_utils/powershell.ps1 and out of win_stat.ps1 so its available to all modules using WANT_POWERSHELL

Observations.

The integration tests look messy now as I have left in commented out tests (which are all just copied from non-windows versions), really so others can review.

I would be grateful for a review of the module documentation as there aren't currently many examples.

I have not been able run the integration tests against window server 2008 or 2008 r2.

In use, care has to be taken when specifying windows paths, especially if you are using a combination of vars and hardcoded paths as
you can easily run into a problem where the generated path
either does not parse as a single quoted string, or if you use double quoted strings, some path names can become mangled because they look like escape sequences.

Using the -v option with ansible-playbook can be very helpful here as it will show the generated paths.

One thing that makes this much easier to handle is that recent versions of windows now accept / or \ for a path separator on windows systems calls.
Anywhere where a path is being passed to powershell, a / seems to be accepted, so in paths for windows modules / will be accepted.
If paths are being passed to older executables this is unlikely to work, however.

For example in the integration tests, the win_output_dir var is specified as

'C:/temp/'

The following variations all fail because some of the tasks specify a directory to copy to without a trailing / or \

don't use:

win_output_dir: 'C:\temp'
win_output_dir: 'C:\temp'
win_output_dir : C:\temp
win_output_dir: C:/temp

And this also fails seeming because the \t part of the name is unescaped to tab character.

don't use either:

win_output_dir: "C:\temp"

ansible-playbook -v output demonstrates this:

TASK: [test_win_file | verify that we are checking a file and it is present] ***
<192.168.99.99> REMOTE_MODULE win_file path=C: emp\foo.txt state=file
failed: [WinTestHost] => {"failed": true}
msg: path will not be created

Currently if you hit this, you will get a stacktrace from (the excellently Pythonically-named) splitter.py

fatal: [WinTestHost] => Traceback (most recent call last):
File "/home/jon/ansible/lib/ansible/runner/init.py", line 582, in _executor
exec_rc = self._executor_internal(host, new_stdin)
File "/home/jon/ansible/lib/ansible/runner/init.py", line 755, in _executor_internal
return self._executor_internal_inner(host, self.module_name, self.module_args, inject, port, complex_args=complex_args)
File "/home/jon/ansible/lib/ansible/runner/init.py", line 988, in _executor_internal_inner
result = handler.run(conn, tmp, module_name, module_args, inject, complex_args)
File "/home/jon/ansible/lib/ansible/runner/action_plugins/win_copy.py", line 253, in run
module_return = self.runner._execute_module(conn, tmp_path, 'win_copy', module_args_tmp, inject=inject, complex_args=complex_args, delete_remote_tmp=delete_remote_tmp)
File "/home/jon/ansible/lib/ansible/runner/init.py", line 504, in _execute_module
argsfile = self._transfer_str(conn, tmp, 'arguments', utils.jsonify(utils.parse_kv(args)))
File "/home/jon/ansible/lib/ansible/utils/init.py", line 781, in parse_kv
vargs = split_args(args)
File "/home/jon/ansible/lib/ansible/module_utils/splitter.py", line 185, in split_args
raise Exception("error while splitting arguments, either an unbalanced jinja2 block or quotes")
Exception: error while splitting arguments, either an unbalanced jinja2 block or quotes

Using -v it is relatively straightforward to avoid however.

Throwing an ansible exception under the circumstances might be sufficient,
although handling the various ways windows paths can be specified would be best.

The initial version of this also had the following notes, which are no longer true:

"I have not tackled moving the checksum logic to sha1 hashes so these modules just use md5 checksums.
Given the large amount of change in these PRs already I felt this was too big a change.

I had to modify the windows version of fetch.py action plugin to get the existing integration tests to pass."

@jhawkesworth
Copy link
Contributor Author

argh, just realised I have merge conflicts. Off to work out how to resolve them.

@jhawkesworth jhawkesworth force-pushed the win_copy_file_template_ansible_core_1 branch from 0d29640 to d2beceb Compare November 25, 2014 23:05
@jhawkesworth
Copy link
Contributor Author

rebased, after a fair bit of git magic.

@cchurch
Copy link
Contributor

cchurch commented Dec 3, 2014

@jhawkesworth From looking at some of your changes, I noticed that the md5 to sha1 updates hadn't been applied for PowerShell, so I'd fixed those in #9688.

My changes there conflicted with some of yours here, so you'll need to rebase this PR.

@jhawkesworth
Copy link
Contributor Author

Thanks for this, once I've rebased the change will be more focused.


if dest.endswith("\\"): # TODO: Check that this fixes the path for Windows hosts.
base = os.path.basename(source)
dest = os.path.join(dest, base)
Copy link
Contributor

Choose a reason for hiding this comment

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

I might suggest use conn.shell.has_trailing_slash and conn.shell.join_path like the copy module for any path manipulations on dest.

@jhawkesworth
Copy link
Contributor Author

I have switched to using conn.shell.has_trailing_slash and conn.shell.join_path in win_template.py.
I have removed the changes I made to fetch.py which were still attempting to work with md5 checksums - fetch is better now anyway following the merge of #9688
I have updated the integration tests (as some contained md5 checksums in asserts).

@jhawkesworth jhawkesworth force-pushed the win_copy_file_template_ansible_core_1 branch 2 times, most recently from f677310 to 3afff1a Compare December 11, 2014 22:03
@jhawkesworth
Copy link
Contributor Author

Rebased again. In the process I noticed that I hadn't actually switched the powershell modules over to using SHA1 checksums, so I have fixed that and pushed against https://ansible/ansible-modules-core#384

@bcoca
Copy link
Member

bcoca commented Dec 16, 2014

Rebase Request

Hi!

Thanks very much for your submission to Ansible. It sincerely means a lot to us.

It looks like the code underneath has changed since this was submitted. Can you take care of rebasing this for us?

  • You should be able to run "git rebase" against this branch to bring it up to date
  • Resolve any merge conflicts and test changes
  • Push to the same branch and github should update the pull request automatically.

Just as a quick reminder of things, this is a really busy project. We have over 800 contributors and to manage the queue effectively
we assign things a priority between P1 (highest) and P5. We'd like to thank you very much for your time!
We'll work things in priority order, so just wanted you to be aware of the queue and know we haven't forgotten about you!

We will definitely see your comments on this issue when reading this ticket, but may not be able to reply promptly. You may also wish to join one of our two mailing lists
which are very active:

Thank you once again for this and your interest in Ansible!

root and others added 2 commits December 16, 2014 04:47
Now uses sha1 checksums following merge of 9688.
Also I undid the changes I made to fetch.py
win_template.py now uses conn.shell.has_trailing_slash and
conn.shell.join_path
updated integration tests.
@jhawkesworth jhawkesworth force-pushed the win_copy_file_template_ansible_core_1 branch from 3afff1a to e37b633 Compare December 16, 2014 06:49
@jhawkesworth
Copy link
Contributor Author

Rebased, as requested.

For what its worth, I had a failure running the new win_feature integration
tests against windows server 2012 R2. I'll gather some more evidence and
see if I can track down the cause.

Jon

On Tue, Dec 16, 2014 at 1:22 AM, Brian Coca notifications@github.com
wrote:

Rebase Request

Hi!

Thanks very much for your submission to Ansible. It sincerely means a lot
to us.

It looks like the code underneath has changed since this was submitted.
Can you take care of rebasing this for us?

  • You should be able to run "git rebase" against this branch to bring
    it up to date
  • Resolve any merge conflicts and test changes
  • Push to the same branch and github should update the pull request
    automatically.

Just as a quick reminder of things, this is a really busy project. We have
over 800 contributors and to manage the queue effectively
we assign things a priority between P1 (highest) and P5. We'd like to
thank you very much for your time!

We'll work things in priority order, so just wanted you to be aware of the
queue and know we haven't forgotten about you!

We will definitely see your comments on this issue when reading this
ticket, but may not be able to reply promptly. You may also wish to join
one of our two mailing lists
which are very active:

Thank you once again for this and your interest in Ansible!


Reply to this email directly or view it on GitHub
#9611 (comment).

bcoca added a commit that referenced this pull request Dec 16, 2014
…ible_core_1

Add support for win_copy, win_file and win_template modules.
@bcoca bcoca merged commit 27d2539 into ansible:devel Dec 16, 2014
@jhawkesworth
Copy link
Contributor Author

Thanks for this. How on earth you guys get through so much stuff I will never know!
All the best,
Jon

** please reply to my unity.demon.co.uk address, not googlemail. Thanks! **

On 16 Dec 2014, at 13:59, Brian Coca notifications@github.com wrote:

Merged #9611.


Reply to this email directly or view it on GitHub.

bcoca added a commit to bcoca/ansible that referenced this pull request Dec 17, 2014
…le_template_ansible_core_1"

This reverts commit 27d2539, reversing
changes made to bfe0856.

Conflicts:
	lib/ansible/modules/core
@ansibot ansibot added feature This issue/PR relates to a feature request. and removed feature_pull_request labels Mar 4, 2018
@ansible ansible locked and limited conversation to collaborators Apr 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature This issue/PR relates to a feature request. P2 Priority 2 - Issue Blocks Release windows Windows community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants