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

Refactor of win_xml (2nd attempt) to add support for processing multiple nodes and counting nodes matched by xpath #53362

Open
wants to merge 5 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@jhawkesworth
Copy link
Contributor

jhawkesworth commented Mar 5, 2019

SUMMARY

This change adds multi-node manipulation, the ability to delete nodes only having matched on the supplied xpath (rather than also the exact expected content at the matched xpath) and add a count capability to win_xml.

The purpose of these changes is to increase the flexibility and utilty of this module, and hopefully make it easier to understand.

I have removed the use of SelectSingleNode so multiple matched elements can be processed in a single run. Now the module works with a list of xml nodes. This should allow using xpaths that match many times inside a single document, for example if you wanted to change lang=en attribute to lang=fr the intention is this should be achievable now.

Also, when type=element or type=attribute and state=absent the module no longer requires exact node match of both the xpath AND the supplied fragment in order to delete nodes. With this modification, only the xpath needs to match. This greatly increases the flexibility of the module as you can use this to remove entire node trees without having to know their exact existing content in advance. This change came from a use case where I wanted to ensure certain nodes did not exist in an nlog.config xml file which allows us to ensure debug output is not written to disk in production environments.

I created a new Save-ChangedXml function to separate the xml manipulation from the point where changes are commited to disk.

Does not fix any bugs but 'scratches an itch' for some features I needed.

ISSUE TYPE
  • Feature Pull Request
COMPONENT NAME

win_xml

ADDITIONAL INFORMATION

Adds a few new tests.
TODO: add tests to demonstrate multi node element and text processing, and multi node delete.

to test, pull the PR contents (perhaps as described here: http://blog.rolpdog.com/2018/08/testing-and-modifying-github-prs.html) and run
source hacking/env-setup
bin/ansible-test windows-integration win_xml -vvv
@jhawkesworth

This comment has been minimized.

Copy link
Contributor Author

jhawkesworth commented Mar 5, 2019

Replaces #53055 which had merge issues.

@ansibot

This comment has been minimized.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 5, 2019

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/windows/win_xml.py:20:161: E501 line too long (171 > 160 characters)

The test ansible-test sanity --test yamllint [explain] failed with 1 error:

test/integration/targets/win_xml/tasks/main.yml:207:1: empty-lines too many blank lines (2 > 0)

click here for bot help

@ansibot ansibot added the ci_verified label Mar 5, 2019

@jhawkesworth

This comment has been minimized.

Copy link
Contributor Author

jhawkesworth commented Mar 5, 2019

I pushed changes to fix the errors identified by CI

@ansibot ansibot removed the ci_verified label Mar 5, 2019

@jhawkesworth

This comment has been minimized.

Copy link
Contributor Author

jhawkesworth commented Mar 6, 2019

currently there's a bug in multiple element change which I working on.

@ansibot ansibot added the stale_ci label Mar 14, 2019

fixed bugs when handling multiple elements, multiple attribute nodes …
…and handling for attribute nodes when using xpaths that only select attributes like //@lang.  Added more tests and tweaked documentation.

@jhawkesworth jhawkesworth changed the title WIP refactor of win_xml (2nd attempt) Refactor of win_xml (2nd attempt) to add support for processing multiple nodes and counting nodes matched by xpath Mar 15, 2019

@jhawkesworth

This comment has been minimized.

Copy link
Contributor Author

jhawkesworth commented Mar 15, 2019

I fixed the issue with multi element change, added more tests and fixed further issues, and tweaked the documentation a little so I think this is ready for consideration.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 15, 2019

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

lib/ansible/modules/windows/win_xml.py:76:161: E501 line too long (172 > 160 characters)

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/windows/win_xml.py:0:0: E305 DOCUMENTATION.notes: not a valid value for dictionary value @ data['notes']. Got 'Only supports operating on xml elements, attributes and text. Namespace, processing-instruction, command and document node types cannot be modified with this module.'

click here for bot help

@jhawkesworth

This comment has been minimized.

Copy link
Contributor Author

jhawkesworth commented Mar 15, 2019

I fixed up the line-too-long error and notes not a list error.

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Mar 15, 2019

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/modules/windows/win_xml.py:76:7: W291 trailing whitespace
lib/ansible/modules/windows/win_xml.py:77:68: W291 trailing whitespace

click here for bot help

@jhawkesworth

This comment has been minimized.

Copy link
Contributor Author

jhawkesworth commented Mar 15, 2019

ready_for_review

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.