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

Implement cleanup command for windows #1155

Merged
merged 1 commit into from
Apr 17, 2020
Merged

Implement cleanup command for windows #1155

merged 1 commit into from
Apr 17, 2020

Conversation

praveenkumar
Copy link
Member

Fixes: Issue #507

Copy link
Contributor

@gbraad gbraad left a comment

Choose a reason for hiding this comment

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

LGTM. Will do a test run

Copy link
Contributor

@gbraad gbraad left a comment

Choose a reason for hiding this comment

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

Works as expected. However, run as admin for the DNS did not show UAC (but this might be related to my config as I disabled the UAC popup at some point). @anjannath can you confirm this?


func removeDnsServerAddress() error {
//Set-DNSClientServerAddress "vEthernet (Default Switch)" -ResetServerAddresses
resetDnsForDefaultSwitchCommand := `Set-DnsClientServerAddress "vEthernet (Default Switch)" -ResetServerAddresses`
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe combine this in a single command to execute, so we don't show two UAC popups.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gbraad I also thought same but may be https://docs.microsoft.com/en-us/powershell/module/dnsclient/set-dnsclientserveraddress?view=win10-ps#syntax InterfaceAlias might work, will need to check.

Copy link
Contributor

Choose a reason for hiding this comment

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

you likely just need to add both commands with a separator. the Alias is an alternative name for the network interface, which "crc" and "default switch" actually are.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gbraad so in this case alias is much better than adding the commands with separator right?

Copy link
Contributor

@gbraad gbraad Apr 15, 2020

Choose a reason for hiding this comment

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

I am not sure what you mean:

PS> Set-DnsClientServerAddress "crc" -ResetServerAddresses`

is the same as:

PS> Set-DnsClientServerAddress -InterfaceAlias "vEthernet (crc)" -ResetServerAddresses

I mean to combine them to execute as:

PS> Set-DnsClientServerAddress "crc" -ResetServerAddresses; Set-DnsClientServerAddress "default switch" -ResetServerAddresses

Copy link
Member Author

Choose a reason for hiding this comment

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

@gbraad How about following?

PS> Set-DnsClientServerAddress -InterfaceAlias ("default switch","crc") -ResetServerAddresses

Copy link
Contributor

Choose a reason for hiding this comment

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

that should be:

PS> Set-DnsClientServerAddress -InterfaceAlias ("vEthernet (Default Switch)","vEthernet (crc)") -ResetServerAddresses

as Alias needs the actual vEthernet prefix. Also note, that "Default Switch" might not be named as such on localized versions of Windows. This is already part of the code and best to use this replacement value.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

which would return nothing if the default does not exist. and could prevent the second device...

but it would still need to be collected in a single statement to prevent the UAC popup IF we by accident have configured it once (before they started to use the one named crc)

Copy link
Member Author

Choose a reason for hiding this comment

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

@gbraad How about following, this way we delete the both and single time UAC popup?

func removeDnsServerAddress() error {
	resetDnsForDefaultSwitchCommand := `Set-DnsClientServerAddress -InterfaceAlias ("vEthernet (crc)") -ResetServerAddresses`
	if exist, defaultSwitch := winnet.GetDefaultSwitchName(); exist {
		resetDnsForDefaultSwitchCommand = fmt.Sprintf(`Set-DnsClientServerAddress -InterfaceAlias ("vEthernet (%s)","vEthernet (crc)") -ResetServerAddresses`, defaultSwitch)
	}
	if _, _, err := powershell.ExecuteAsAdmin("Remove dns entry for default switch", resetDnsForDefaultSwitchCommand); err != nil {
		return err
	}
	return nil
}

@anjannath
Copy link
Member

anjannath commented Apr 15, 2020

@gbraad am getting two UAC prompts when it tries and removes the dns entries.

@@ -166,3 +167,39 @@ func checkIfRunningAsNormalUser() error {
logging.Debug("Ran as administrator")
return fmt.Errorf("crc should be ran as a normal user")
}

func removeDnsServerAddress() error {
resetDnsForDefaultSwitchCommand := `Set-DnsClientServerAddress -InterfaceAlias ("vEthernet (crc)") -ResetServerAddresses`
Copy link
Contributor

Choose a reason for hiding this comment

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

default switch?

func removeDnsServerAddress() error {
resetDnsForDefaultSwitchCommand := `Set-DnsClientServerAddress -InterfaceAlias ("vEthernet (crc)") -ResetServerAddresses`
if exist, defaultSwitch := winnet.GetDefaultSwitchName(); exist {
resetDnsForDefaultSwitchCommand = fmt.Sprintf(`Set-DnsClientServerAddress -InterfaceAlias ("vEthernet (%s)","vEthernet (crc)") -ResetServerAddresses`, defaultSwitch)
Copy link
Contributor

Choose a reason for hiding this comment

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

why not insert the string for the default, by appending...

or collect the options and return the -InterfaceAlias ("vEthernet (crc)", ... accordingly.

Copy link
Contributor

Choose a reason for hiding this comment

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

resetDnsForDefaultSwitchCommand is not the right name when only crc is used.

Rather call this resetDnsForVirtualSwitchCommand

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

Successfully merging this pull request may close these issues.

3 participants