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

Conversation

floradu88
Copy link

Hey @pmario,

I've created some fixes for hyper-v. Could you please take a look at this?

I am doing some more tests after I create some machines to test all the test cases.

Regards,
Radu

radu.florescu and others added 13 commits February 11, 2016 17:37
Added ability to work with Hyper-v
Merge latest changes to my fork
…s installed.

Signed-off-by: radu florescu <radu.florescu@hotmail.com>
Signed-off-by: radu.florescu <radu.florescu@totaljobsgroup.com>
Signed-off-by: radu.florescu <radu.florescu@totaljobsgroup.com>
- when hyper-v exists is present and not virtualbox.
- when both hyper-v and virtualbox are missing.

Signed-off-by: radu.florescu <radu.florescu@hotmail.com>
}).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.

…on windows 10.

Signed-off-by: raduflorescu <radu.florescu@hotmail.com>
…e, but we can checkk for `fullyqualifiederrorid`.

Added Machine name when checking docker machine version.

Signed-off-by: raduflorescu <radu.florescu@hotmail.com>
@floradu88
Copy link
Author

floradu88 commented Apr 15, 2016

By non-admin I meant user that has been added to the hyper-v group or users and relogged in.

So I've tested the following cases:

  1. Admin user, Hyperv, no virtual box -> works with Hyperv
  2. Admin user, Hyperv, virtual box -> works with Hyperv, no vbox is created
  3. Non-Admin user, Hyperv, no virtualbox -> works with Hyperv
  4. Non-Admin user, Hyperv, virtualbox -> works with Hyperv, no vbox is created

I am uninstalling hyper-v and installing virtualbox only, stay tuned :)

@pmario
Copy link
Owner

pmario commented Apr 15, 2016

ahh. I understand now, what you want to achieve. .... But IMO it's not possible to have hyper-v and VBox active at the same time.

@floradu88
Copy link
Author

floradu88 commented Apr 15, 2016

Yes, but I still want to cover all the test cases.

Looks like virtualbox has some minor issues. And vbox 5.0.16 is not good, it fails to create the hard disk drive.

Added defensive programming in accessing vbox.log

Signed-off-by: raduflorescu <radu.florescu@hotmail.com>
@floradu88
Copy link
Author

floradu88 commented Apr 15, 2016

I really can't test virtualbox, looks like 5.0.14(default installed version for DockerToolbox) is also not working.

@pmario @FrenchBen Could you please take a look at virtual box with this last commit?


let match = stdout.match(/^(.*) True/im);
return util.execFile([this.command(), 'Get-VM | Where {$_.Name -eq "'+ machineName +'" -and $_.Version} | select Version']).then(stdout => {
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.

@pmario
Copy link
Owner

pmario commented Apr 18, 2016

@floradu88 .. I'll pull it and add some more changes.

@floradu88
Copy link
Author

floradu88 commented Apr 18, 2016

@pmario or you could merge it, then add your changes on top.

radu.florescu added 2 commits April 18, 2016 15:56
Signed-off-by: radu.florescu <radu.florescu@hotmail.com>
…not vbox is installed, because there is an exception thrown in the setup earlier.

Signed-off-by: radu.florescu <radu.florescu@hotmail.com>
@pmario pmario merged commit 9cc57e5 into pmario:add-hyperv Apr 18, 2016
@pmario
Copy link
Owner

pmario commented Apr 18, 2016

@floradu88 I did merge your stuff added mine and pushed to the upstream PR again. ... I hope I didn't mess it up :)

@pmario
Copy link
Owner

pmario commented Apr 18, 2016

The whole thing imo contains some pretty ugly CLI commands. .... and I really have a hard time, with the switch configurations. ... For hyper-v we would need a splash screen with some actual config settings as the very first screen. So kitematic shouldn't start, until the settings are confirmed :/

@FrenchBen
Copy link

@pmario that's very much in-line with some machine management. Perhaps we need to get some thoughts together on this feature and how it should work, then move forward with it.

@floradu88
Copy link
Author

floradu88 commented Apr 19, 2016

@pmario and @FrenchBen I am going to start adding window to create a new virtual switch from within Hyperv with ability to change the connection attached to it.

@pmario
Copy link
Owner

pmario commented Apr 19, 2016

@floradu88 IMO this should be a new PR starting from the master branch. I think it can be a stand alone configuration. .. I personally would like some thing like a splash screen with some info eg:

  • we found provider X, Y, Z ... VBox, Hyper-V, VMWare ...
    • which one do you want to use
  • if Hyper-V is selected .. new page: We found 3 switches
    • which one do you want to use
  • memory usage
  • other possible machine params ... all of them

.. and so on.

@floradu88
Copy link
Author

floradu88 commented Apr 20, 2016

@pmario OK, new PR for this. Btw we need to also do some changes to docker/toolbox in order to change the start.sh script into powershell

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