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

govc: fix default guest.run path for unsupported Windows guests #2451

Merged
merged 1 commit into from
May 25, 2021

Conversation

dougm
Copy link
Member

@dougm dougm commented May 20, 2021

  • Add '-w' flag to guest.run

Closes: #2446

@embano1
Copy link
Contributor

embano1 commented May 21, 2021

Thx Doug, I'm fine with the code changes but was wondering why we don't allow to overwrite the SHELL but depend on the OS type instead? Wouldn't changing the SHELL be more flexible instead of introducing that new windows specific parameter?

@dougm
Copy link
Member Author

dougm commented May 21, 2021

@embano1 we can override if an absolute path is given, for example:

% govc guest.run -vm foo /usr/local/bin/any-program ...

The os family based bash -c and cmd.exe /c defaults when non-absolute path is given is just for convenience.
We could live without the new -w flag, in this case that would mean having to specify:

% govc guest.run -vm foo c:\\Windows\\System32\\cmd.exe /c powershell c:\\bar.ps1

Instead of:

% govc guest.run -vm foo -w powershell c:\\bar.ps1

@embano1
Copy link
Contributor

embano1 commented May 21, 2021

Thx for confirming my assumption that this is possible today, but adding -w is a convenience feature. Do you foresee other combinations or more flags where this would be needed or is Windows the exception here?

@dougm
Copy link
Member Author

dougm commented May 21, 2021

Yeah, windows is the exception. Instead of adding -w, we could instead do the change below, toolsInstallType would only ever be MSI on Windows guests. What do you think?

modified   govc/vm/guest/guest.go
@@ -88,12 +88,18 @@ func (flag *GuestFlag) Toolbox() (*toolbox.Client, error) {
 
 	family := ""
 	var props mo.VirtualMachine
-	err = vm.Properties(context.Background(), vm.Reference(), []string{"guest.guestFamily"}, &props)
+	err = vm.Properties(context.Background(), vm.Reference(), []string{"guest.guestFamily", "guest.toolsInstallType"}, &props)
 	if err != nil {
 		return nil, err
 	}
 	if props.Guest != nil {
 		family = props.Guest.GuestFamily
+		if family == string(types.VirtualMachineGuestOsFamilyOtherGuestFamily) {
+			if props.Guest.ToolsInstallType == string(types.VirtualMachineToolsInstallTypeGuestToolsTypeMSI) {
+				// The case of Windows version not supported by the ESX version
+				family = string(types.VirtualMachineGuestOsFamilyWindowsGuest)
+			}
+		}
 	}
 
 	return &toolbox.Client{

@embano1
Copy link
Contributor

embano1 commented May 21, 2021

Nice, I like that much better! No (breaking) change for user and it will behave accordingly to what users would expect (see issue).

@dougm
Copy link
Member Author

dougm commented May 23, 2021

ok, done @embano1 👍

govc/vm/guest/run.go Outdated Show resolved Hide resolved
guest/toolbox/client.go Outdated Show resolved Hide resolved
- Default to Windows family when family is "Other" and toolsInstallType is MSI

Fixes vmware#2446
Copy link
Contributor

@embano1 embano1 left a comment

Choose a reason for hiding this comment

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

LGTM

@dougm dougm merged commit 99201d0 into vmware:master May 25, 2021
@dougm dougm deleted the guest-run-other branch May 25, 2021 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Executing file on windows vm using govc
3 participants