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

win_hosts to use Ansible.Basic CSharp Util and better diff support #58600

Open
wants to merge 3 commits into
base: devel
from

Conversation

@mhunsber
Copy link
Contributor

commented Jul 1, 2019

SUMMARY

Modifies win_hosts module to use the Ansible.Basic CSharp Util instead of the legacy powershell module.
Also includes an improvement to the diff support. Instead of generating a prepared diff, it uses the before, after properties.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

win_hosts

ADDITIONAL INFORMATION

I also updated the documents so that it better matches the development guide.

before diff change

TASK [win_hosts : remove aliases from the list] ***********

-[192.168.168.1 testhost alias1 alias2 alias3 alias4]
+[192.168.168.1 testhost alias1 alias2 ]

after diff change

TASK [win_hosts : remove aliases from the list] ***********

@@ -19,4 +19,4 @@
 # localhost name resolution is handled within DNS itself.
 #      127.0.0.1       localhost
 #      ::1             localhost
-192.168.168.1 testhost alias1 alias2 alias3 alias4
+192.168.168.1 testhost alias1 alias2
@ansibot

This comment has been minimized.

@jborean93
Copy link
Contributor

left a comment

The code looks great, nice to see it being simplified. Just some minor nits on the documentation changes.

- Modifies %windir%\system32\drivers\etc\hosts.
- Maps IPv4 or IPv6 addresses to canonical names.
- Adds, removes, or sets cname records for ip and hostname pairs.
- Modifies %windir%\\system32\\drivers\\etc\\hosts.

This comment has been minimized.

Copy link
@jborean93

jborean93 Jul 4, 2019

Contributor

You shouldn't need to double up on \ in the docs. The Python doc string is prefixed with r so no escaping necessary.

This comment has been minimized.

Copy link
@mhunsber

mhunsber Jul 4, 2019

Author Contributor

Interesting, I thought that was why on the website the \s aren't showing up.
https://docs.ansible.com/ansible/latest/modules/win_hosts_module.html#synopsis

This comment has been minimized.

Copy link
@jborean93

jborean93 Jul 4, 2019

Contributor

Looks like a potential bug in our doc generation for synopsis, we can see the values in the module options table are fine based on win_copy https://github.com/ansible/ansible/blob/devel/lib/ansible/modules/windows/win_copy.py#L39. Let's leave it as you have it for now as it's better to have at least something there even if it changes in the future.

This comment has been minimized.

Copy link
@mhunsber

mhunsber Jul 9, 2019

Author Contributor

Sounds good. I tested the HTML output with the extra \s using make webdocs and the synopsis displayed correctly.
output:

<ul class="simple">
<li>Manages hosts file entries on Windows.</li>
<li>Maps IPv4 or IPv6 addresses to canonical names.</li>
<li>Adds, removes, or sets cname records for ip and hostname pairs.</li>
<li>Modifies %windir%\system32\drivers\etc\hosts.</li>
</ul>
Show resolved Hide resolved lib/ansible/modules/windows/win_hosts.py Outdated
Show resolved Hide resolved lib/ansible/modules/windows/win_hosts.py Outdated

@ansibot ansibot added needs_revision and removed core_review labels Jul 4, 2019

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.