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

Fix Vagrantfile's Windows RAM calculation #3206

Merged
merged 2 commits into from Feb 15, 2020
Merged

Conversation

ocdtrekkie
Copy link
Collaborator

@ocdtrekkie ocdtrekkie commented Feb 7, 2020

See sandstorm-io/vagrant-spk#241 for the same fix on vagrant-spk, see sandstorm-io/vagrant-spk#240 for details on how the existing command responds (listing half the RAM twice instead of providing a single valid amount of RAM).

See sandstorm-io/vagrant-spk#241 for the same fix on vagrant-spk, see sandstorm-io/vagrant-spk#240 for details on how the existing command responds (listing half the RAM twice instead of providing a single valid amount of RAM).

But in short, the PowerShell command
@ocdtrekkie ocdtrekkie added the sandstorm-dev Issues hacking on Sandstorm label Feb 7, 2020
@ocdtrekkie
Copy link
Collaborator Author

As an aside (since I'd want to do it in vagrant-spk too/first)... I am really tempted to remove the rescue block. What on earth are we doing with code designed to prevent failure on XP and Vista?

@zenhack
Copy link
Collaborator

zenhack commented Feb 7, 2020

I'm fine with junking the rescue, given that both of those OSes are EOL.

@ocdtrekkie
Copy link
Collaborator Author

I'll test removing it and then add that change here and to vagrant-spk. There are still people adamantly using Windows 7, but I know of nobody militantly attached to Windows Vista. Furthermore, as I noted in the vagrant-spk repo, I believe this bug should error out because the PowerShell is wrong, but the rescue block likely was suppressing the fact that this code was broken.

@ocdtrekkie
Copy link
Collaborator Author

Tested vagrant-spk with the original broken version which gave me a 512 MB RAM VM (the default), and with the corrected command and the rescue block removed, and got a 4 GB RAM VM (a quarter of my system RAM).

Effectively drops support for Windows XP/Vista from Vagrant installs of Sandstorm, but fixes an issue where broken code here does not error out and simply fails silently.
@ocdtrekkie ocdtrekkie added the ready-for-review We think this is ready for review label Feb 10, 2020
@kentonv
Copy link
Member

kentonv commented Feb 15, 2020

I have no idea what this does but I trust you. :)

@kentonv kentonv merged commit 9be8935 into master Feb 15, 2020
@kentonv kentonv deleted the fix-vagrantfile-windows-ram branch February 15, 2020 19:04
@ocdtrekkie ocdtrekkie removed the ready-for-review We think this is ready for review label Feb 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sandstorm-dev Issues hacking on Sandstorm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants