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

Attributes are not added to node if it already exists but has no attributes #18

Closed
rpolley opened this issue Oct 29, 2020 · 7 comments · Fixed by #20
Closed

Attributes are not added to node if it already exists but has no attributes #18

rpolley opened this issue Oct 29, 2020 · 7 comments · Fixed by #20
Assignees
Labels

Comments

@rpolley
Copy link

rpolley commented Oct 29, 2020

I was trying to use this to edit some xml based config files, but I ran into an issue: consider the following xml file somefile.xml

<config>
  <setting>
</config>

And the following manifest

xml_fragment { 'set setting.value to 1` :
  ensure => 'present'
  path => 'somefile.xml',
  xpath => '/config/setting',
  content => {
    attributes => {
      value => 1
    }
  }

Will result in an unchanged somefile.xml, however if the somefile.xml file was instead:

<config>
  <setting value=2>
</config>

You would instead end up with

<config>
  <setting value=1>
</config>

This is of course a simplified example, but it causes bugs when you try to use this to edit IIS config files, which use the convention of having attributes represent leaf settings that can be assigned values and the absence of a given attribute means that the setting the attribute represents takes its default value.

@rpolley
Copy link
Author

rpolley commented Oct 29, 2020

I believe this is caused by

next unless should.key?('attributes') && i.key?('attributes')
which causes the provider matches method to not compare the should value for the attributes to the is value of the attributes if the is value is nil, which is the case if the tag exists but does not have attributes

@Areson Areson self-assigned this Oct 29, 2020
@Areson Areson added the bug label Oct 29, 2020
@Areson
Copy link
Owner

Areson commented Oct 29, 2020

I will try and review this in the next week.

Areson added a commit that referenced this issue Nov 8, 2020
Per #18 adding check to see if existing node has no attributes but definition does. This was a case that the last round of revisions caused to be missed.
@Areson
Copy link
Owner

Areson commented Nov 8, 2020

@rpolley You were spot on. Adding a check for the case where the definition has attributes defined but the existing element doesn't have any. Tested it on my end and it appears to work, but go ahead and pull from the development branch and give if a go before I package it up.

@Areson
Copy link
Owner

Areson commented Nov 20, 2020

@rpolley Bump

@1WindowsPuppetAdmin
Copy link

@Areson - can you merge this in? i don't think we need to wait on rpolley as long as you tested it and it works. I've been waiting for this fix as well.

@rpolley
Copy link
Author

rpolley commented Feb 22, 2021

@Areson seconding this, @1WindowsPuppetAdmin is probably in a better position than I am to validate that this works anyway.

@Areson Areson mentioned this issue Mar 9, 2021
@Areson Areson linked a pull request Mar 9, 2021 that will close this issue
@Areson Areson closed this as completed in #20 Mar 9, 2021
@Areson
Copy link
Owner

Areson commented Mar 9, 2021

I've released the updated version on the forge. Thanks for your patience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants