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_firewall_rule: Only report changed when change is made #57267

Open
wants to merge 4 commits into
base: devel
from

Conversation

Projects
None yet
4 participants
@ShachafGoldstein
Copy link
Contributor

commented Jun 1, 2019

SUMMARY

Fix bug #44450

ISSUE TYPE
  • Bugfix Pull Request
COMPONENT NAME

win_firewall_rule.ps1

ADDITIONAL INFORMATION
@ansibot

This comment has been minimized.

@samdoran samdoran removed the needs_triage label Jun 4, 2019

@samdoran samdoran changed the title win_firewall_rule: Fix bug https://github.com/ansible/ansible/issues/44450 win_firewall_rule: Only report changed when change is made Jun 4, 2019

@@ -156,7 +156,7 @@ try {
# the default for enabled in module description is "true", but the actual COM object defaults to "false" when created
if ($null -ne $enabled) { $new_rule.Enabled = $enabled } else { $new_rule.Enabled = $true }
if ($null -ne $description) { $new_rule.Description = $description }
if ($null -ne $program -and $program -ne "any") { $new_rule.ApplicationName = $program }
if ($null -ne $program -and $program -ne "any") { $new_rule.ApplicationName = [System.Environment]::ExpandEnvironmentVariables($program) }

This comment has been minimized.

Copy link
@jborean93

jborean93 Jun 10, 2019

Contributor

Instead of always setting the value as the expanded environment var we should probably just check the expanded version, or someone get the raw string back on our idempotency checks. This still allows people to specify the program using an expand string and we check it properly.

Also having a test for this scenario added to our integration tests would be great.

This comment has been minimized.

Copy link
@ShachafGoldstein

ShachafGoldstein Jun 10, 2019

Author Contributor

The only place I see that we can expand the path is on line 185 inside the $userPassedArguments array

This comment has been minimized.

Copy link
@jborean93

jborean93 Jun 10, 2019

Contributor

I haven't looked at the code so no idea what the COM function returns on our checks. If that auto expands the value then we should do that on our checks. If the value is actually stored as an expanded string, i.e. the GUI doesn't show the expand string value, then maybe what you have here is correct. Really just needs some more investigation.

This comment has been minimized.

Copy link
@ShachafGoldstein

ShachafGoldstein Jun 10, 2019

Author Contributor

I checked it out before the PR, it saves it as expanded that was the source of the issue

This comment has been minimized.

Copy link
@ShachafGoldstein

ShachafGoldstein Jun 10, 2019

Author Contributor

This change fail, only expanding in 159 seems to do the trick.

our other option is adding all kinds of IFs in the code to expand properties where they relate to applicationname

@ansibot

This comment has been minimized.

Copy link
Contributor

commented Jun 10, 2019

cc @TimothyVandenbrande @ar7z1
click here for bot help

@ansibot ansibot removed the stale_ci label Jun 10, 2019

ShachafGoldstein added some commits Jun 10, 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.