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

Fixed usages from powershell commands #1

Merged
merged 18 commits into from
Apr 18, 2016
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 14 additions & 18 deletions src/utils/DockerMachineUtil.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import path from 'path';
import Promise from 'bluebird';
import fs from 'fs';
import util from './Util';
import powershellUtil from './PowershellUtil';
import child_process from 'child_process';

var DockerMachine = {
Expand All @@ -13,9 +14,6 @@ var DockerMachine = {
return '/usr/local/bin/docker-machine';
}
},
commandElevated: function () {
return 'powershell.exe';
},
name: function () {
return 'default';
},
Expand Down Expand Up @@ -60,31 +58,29 @@ var DockerMachine = {
return false;
});
},
create: function (machineName = this.name(), provider) {
create: function (machineName = this.name(), provider = "hyperv", virtualBoxIsInstalled = true) {
Copy link
Owner

Choose a reason for hiding this comment

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

imo there is no need for the extra check of virtualboxinstalled. create is only called, if we are already sure, that we can call it.

Copy link
Author

Choose a reason for hiding this comment

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

virtualboxinstalled is only needed as at that step you might not have it installed and we need to through an Exception and stop the whole process.

//TODO: check options elements!
switch (provider){
case "virtualbox":{
console.log('started create with virtualbox');
return util.execFile([this.command(), '-D', 'create', '-d', 'virtualbox', '--virtualbox-memory', '2048', machineName]);
}
case "hyperv":{
//The switch may contain spaces!
console.log('started create with hyper-v');
let virtualSwitch = localStorage.getItem('virtualSwitch');
console.log('will use this switch ' + virtualSwitch + ' with external')

let args= `"` + `docker-machine.exe -D create --driver hyperv --hyperv-memory 2048 `+
`--hyperv-virtual-switch '${virtualSwitch}' ${machineName}` + `"`;

//TODO: in an ideal world, powershell has its own PowershellUtil.js ;)
console.log("cmd: ", args)
return util.execFile([this.commandElevated(), 'start-process', 'powershell', '-verb', 'runas', '-wait', '-argumentList', args]).then(stdout => {
return Promise.resolve(null);
}).catch((error) => {
throw new Error(error.message);
});
return powershellUtil.runCommandWithArgs(args);
}
default:{
default: {
console.log("virtualboxInstalled: " + virtualBoxIsInstalled);
if (virtualBoxIsInstalled === true) {
console.log('started create with virtualbox');
return util.execFile([this.command(), '-D', 'create', '-d', 'virtualbox', '--virtualbox-memory', '2048', machineName]);

let args= `"docker-machine.exe -D create --driver virtualbox --virtualbox-memory 2048 ` + machineName + `"`;
return powershellUtil.runCommandWithArgs(args);
} else {
throw new Error('virtualbox not installed, so this cannot continue');
}
}
}
},
Expand Down
21 changes: 9 additions & 12 deletions src/utils/HypervBoxUtil.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,6 @@ var HypervBox = {
return 'powershell.exe';
},
// TODO: ..We'll probably need a differnt command here in the future. That's why code dupe.
commandElevated: function () {
return 'powershell.exe';
},
installed: function () {
return util.execFile([this.command(), '@(Get-Command get-vm).ModuleName']).then(stdout => {
console.log('stdout: ', stdout);
Expand All @@ -31,19 +28,19 @@ var HypervBox = {
*/

hasAdminRights: function() {
return util.execFile([this.command(), 'Get-VMHostSupportedVersion']).then(stdout => {
return util.execFile([this.command(), 'Get-VMHost']).then(stdout => {
Copy link
Owner

@pmario pmario Apr 15, 2016

Choose a reason for hiding this comment

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

If I run the command get-vmhost from a non elevated powershell I get this output:

get-vmhost : Sie besitzen nicht die erforderliche Berechtigung für diese Aufgabe. Wenden Sie sich an den Administrator
der Autorisierungsrichtlinie für Computer "PC-MARIO".
In Zeile:1 Zeichen:1
+ get-vmhost
+ ~~~~~~~~~~
    + CategoryInfo          : NotSpecified: (:) [Get-VMHost], VirtualizationException
    + FullyQualifiedErrorId : Unspecified,Microsoft.HyperV.PowerShell.Commands.GetVMHost

So for me there is no text "invalidoperation" ...

Copy link
Author

Choose a reason for hiding this comment

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

Interesting, so maybe FullyQualifiedErrorId would be better to check for.

Copy link
Author

Choose a reason for hiding this comment

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

@pmario changed this in my latest commit.

console.log('stdout: ', stdout);
// To execute the above command you'll need Admin or Hyper-V-Admin rights.
if ( stdout.toUpperCase().indexOf('TRUE') !== -1) {
if (stdout.toLowerCase().indexOf('invalidoperation') === -1) {
Copy link
Owner

Choose a reason for hiding this comment

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

did you have a problem with this admin test command?

Copy link
Author

Choose a reason for hiding this comment

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

That function is not available for all installations. It is present on 50% of the machines I've tested.

Copy link
Owner

Choose a reason for hiding this comment

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

ah. ok

Copy link
Author

Choose a reason for hiding this comment

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

I haven't pinpointed which package/program added the powershell commands :(

Copy link
Owner

Choose a reason for hiding this comment

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

just found

PS C:\Users\Mario> get-command get-vmhost

CommandType     Name                                               Version    Source
-----------     ----                                               -------    ------
Cmdlet          Get-VMHost                                         2.0.0.0    Hyper-V


PS C:\Users\Mario> get-module -ListAvailable -Name Hyper-v


    Verzeichnis: C:\WINDOWS\system32\WindowsPowerShell\v1.0\Modules


ModuleType Version    Name                                ExportedCommands
---------- -------    ----                                ----------------
Binary     2.0.0.0    Hyper-V                             {Add-VMAssignableDevice, Add-VMDvdDrive, Add-VMFibreChannelHba, Add-VMGroupMember...}
Binary     1.1        Hyper-V                             {Add-VMDvdDrive, Add-VMFibreChannelHba, Add-VMHardDiskDrive, Add-VMMigrationNetwork...}


PS C:\Users\Mario>

As you suggested. FullyQualifiedErrorId may be a good option

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the above lines of code.

return Promise.resolve(true);
}
return Promise.resolve(false);
}).catch(() => {
return Promise.resolve(false);
});
},
switchName: function (name) {
// return util.execFile([this.command(), '$(Get-VMSwitch | where {$_.SwitchType -eq "external"}).name']).then(out => {
return util.execFile([this.command(), '$(Get-VMSwitch).name']).then(out => {
return util.execFile([this.command(), '$(Get-VMSwitch | where {$_.SwitchType -eq "external"}).name']).then(out => {
Copy link
Owner

Choose a reason for hiding this comment

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

We can't use the test for an external switch, since it's possible to use internal switches, too! An internal swtich can get internet access with a shared network connection. see: docker#1541 (comment)

So we basically need to use the first one, we find. .... even if it is the wrong one.

Copy link
Author

Choose a reason for hiding this comment

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

@pmario this was removed in the latest version of code, after extensive testing on several machines.

// We use the same mechanism as docker-machine. Use the first switch we find.
return (out.replace('\r','').split('\n')[0]);
}).catch(() => {
Expand All @@ -54,9 +51,8 @@ var HypervBox = {
// there seems to be a problem with elevated execution. see: https://github.com/nodejs/node-v0.x-archive/issues/6797
// The only easy possibility seems to be to communicat with a file.

return util.execFile([this.command(), 'Get-VMHostSupportedVersion']).then(stdout => {

let match = stdout.match(/^(.*) True/im);
return util.execFile([this.command(), 'Get-VM | Where {$_.Name -eq "default" -and $_.Version} | select Version']).then(stdout => {
Copy link
Owner

Choose a reason for hiding this comment

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

imo default shouldn't be hard coded. We may use the "name" variable instead.

Copy link
Author

Choose a reason for hiding this comment

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

Will change this to be name instead of default

Copy link
Author

Choose a reason for hiding this comment

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

This might be done as a future pull request as default if hard coded all over the place now.

Copy link
Author

Choose a reason for hiding this comment

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

@pmario changed this in my latest commit. Now using machine.name()

let match = stdout.match(/\d+(?:\.\d+)+/g);
Copy link
Owner

@pmario pmario Apr 18, 2016

Choose a reason for hiding this comment

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

Did test the stuff. Seems to work with hyper-v,

but there is a problem here. needs to be match[0] since we only have one version number. and it seems the promise chain is broken here. ... need some more investigation

Copy link
Author

@floradu88 floradu88 Apr 18, 2016

Choose a reason for hiding this comment

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

match[0] is correct, I added an extra check to see if we get one or more versions. Check my latest commit.

if (match != null) {
// matched text: match[0]
// match start: match.index
Expand All @@ -76,8 +72,9 @@ var HypervBox = {
// Needed for consistency check
vmExists: function (name) {
return util.execFile([this.command(), "Get-VM | Where {$_.Name -eq '" + name + "'}"]).then(out => {
console.log(out)
return (out.indexOf(name) !== -1);
console.log(out);
var machineExists = out.indexOf(name) !== -1;
return machineExists;
}).catch(() => {
return false;
});
Expand Down
18 changes: 18 additions & 0 deletions src/utils/PowershellUtil.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import util from './Util';

var PowershellUtil =
{
runCommandWithArgs : function (args) {
console.log("cmd: ", args);
return util.execFile([this.commandElevated(), 'start-process', 'powershell', '-verb', 'runas', '-wait', '-argumentList', args]).then(stdout => {
return Promise.resolve(null);
}).catch((error) => {
throw new Error(error.message);
});
},
commandElevated: function commandElevated() {
return 'powershell.exe';
},
};

module.exports = PowershellUtil;
2 changes: 1 addition & 1 deletion src/utils/SetupUtil.js
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ export default {
*
*
*/
await machine.create(machine.name(), provider);
await machine.create(machine.name(), provider, virtualBoxInstalled);
} else {
let state = await machine.status();
if (state !== 'Running') {
Expand Down