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

initial xml module/state #49822

Merged
merged 10 commits into from
Oct 19, 2018
Merged

initial xml module/state #49822

merged 10 commits into from
Oct 19, 2018

Conversation

mchugh19
Copy link
Contributor

@mchugh19 mchugh19 commented Sep 28, 2018

What does this PR do?

Adds new xml module to query/modify basic xml elements from an XPath query

New Behavior

Given the file:

<root xmlns:foo="http://www.foo.org/" xmlns:bar="http://www.bar.org">
	<actors>
		<actor id="1">Christian Bale</actor>
		<actor id="2">Liam Neeson</actor>
		<actor id="3">Michael Caine</actor>
	</actors>
	<foo:singers>
		<foo:singer id="4">Tom Waits</foo:singer>
		<foo:singer id="5">B.B. King</foo:singer>
		<foo:singer id="6">Ray Charles</foo:singer>
	</foo:singers>
</root>

The following functionality is implemented:
get/set attributes

# salt-call xml.get_attribute /tmp/test.xml ".//actor[@id='3']"
local:
    ----------
    id:
        3

# salt-call xml.set_attribute /tmp/test.xml ".//actor[@id='3']" edited "uh-huh"
local:
    True

# salt-call xml.get_attribute /tmp/test.xml ".//actor[@id='3']"
local:
    ----------
    edited:
        uh-huh
    id:
        3

get/set values

# salt-call xml.get_value /tmp/test.xml ".//actor[@id='2']"
local:
    Liam Neeson

# salt-call xml.set_value /tmp/test.xml ".//actor[@id='2']" "Patrick Stewart"
local:
    True

# salt-call xml.get_value /tmp/test.xml ".//actor[@id='2']"
local:
    Patrick Stewart

It's pretty simplistic, but if this seems to be a good direction to go, it can form the foundation for a matching xml state to search/replace values.

Tests written?

Yes

Commits signed with GPG?

No

Copy link
Contributor

@cachedout cachedout left a comment

Choose a reason for hiding this comment

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

I like this! I do think that we should add information about this to the release notes prior to merging it, however.

@cachedout
Copy link
Contributor

Additionally, there are two lint errors that need to be fixed before we can merge this:

https://jenkinsci.saltstack.com/job/pr-lint/job/PR-49822/3/warnings52Result/new/

@mchugh19
Copy link
Contributor Author

Short blurb added to the release notes. However, if it's release note worthy, then this should really have an accompanying state to perform file.line or file.replace type functions. I'll take a stab at that when able.

Christian McHugh added 2 commits October 1, 2018 06:56
gracefully fail state if xpath doesn't exist
@mchugh19 mchugh19 changed the title initial xml module initial xml module/state Oct 1, 2018
@dwoz
Copy link
Contributor

dwoz commented Oct 3, 2018

Great work here. Any chance we can get some test coverage for this?

@mchugh19
Copy link
Contributor Author

mchugh19 commented Oct 3, 2018

@dwoz there are tests on the module. Do you mean the state as well? As it just calls the module, do you have a suggestion?

@rallytime
Copy link
Contributor

@mchugh19 Yes, state tests would be very welcome. :)

Looks like there is also a merge conflict here with the release notes. Can you rebase this when you're ready with some more tests?

@mchugh19
Copy link
Contributor Author

mchugh19 commented Oct 6, 2018

I think this is all set. Test and docs should be complete

@rallytime rallytime requested review from cachedout and dwoz October 8, 2018 15:12
@cachedout
Copy link
Contributor

@dwoz Can you come by and review this, please? Thanks.

@rallytime
Copy link
Contributor

@dwoz bump :)

@cachedout cachedout merged commit efd2365 into saltstack:develop Oct 19, 2018
@mchugh19 mchugh19 deleted the xml_module branch July 4, 2019 14:23
mchugh19 pushed a commit to mchugh19/salt that referenced this pull request Oct 12, 2019
@waynew waynew added the has master-port port to master has been created label Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has master-port port to master has been created
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants