search

ansible-collections / community.windows

Windows community collection for Ansible
https://galaxy.ansible.com/community/windows
GNU General Public License v3.0
193 stars 152 forks source link

Update win_xml.ps1 #520

Closed bct-timo-crabbe closed 11 months ago

bct-timo-crabbe commented 1 year ago
SUMMARY

Fixed bug: module would not add new child nodes to empty parent nodes

Moved code that will modify the xml node outside the check to find an existing node

ISSUE TYPE
COMPONENT NAME

module: win_xml

bct-timo-crabbe commented 1 year ago

I added the changelog fragment as you requested. There already are integration tests for this bug in the file you mention, lines 103-127.

jborean93 commented 1 year ago

There already are integration tests for this bug in the file you mention, lines 103-127.

If there are already tests for this then how are they not failing? Sounds like the tests aren't covered the bug and more tests are needed.

bct-timo-crabbe commented 1 year ago

It looks like this bug was already fixed by pull request #482. The way this bug is fixed in that pull request is applied in a very roundabout way, I would prefer to apply my solution. What would be the best way to go forward? Should I update the changelog fragment to minor_changes and describe the change I have done?

jborean93 commented 1 year ago

I guess the question is for me, what's the benefits of updating the code if it's already fixed. Are there further fixes with your code here, is there some problems with the other PR.

bct-timo-crabbe commented 1 year ago

The fix in the other PR does not fix the root cause of the problem, it only forces the code to execute properly by adding extra logic that isn't necessary. There would be some performance benefits, but these would be negligible.

jborean93 commented 1 year ago

The fix in the other PR does not fix the root cause of the problem, it only forces the code to execute properly by adding extra logic that isn't necessary.

From my perspective it fixed the issue, you even stated there is already a test for it that was added by the PR and it passes. My concern is that while your fix might be better the other fix might also solve other problems. Unless there is a clear reason why reverting the previous PR will help I'm not sure I can accept this PR sorry.

github-actions[bot] commented 11 months ago

This pull request is stale because it has been open for 4 weeks with no activity. Remove stale label or comment or this will be closed in 2 weeks.