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

virtualbox.{nix,py}: add .vcpu option #540

Merged
merged 2 commits into from
Jan 9, 2017

Conversation

Cireo
Copy link
Contributor

@Cireo Cireo commented Nov 2, 2016

We follow the naming convention used by libvirt, and convert this
option to a "--cpu" command line option for VBoxManage.

@domenkozar
Copy link
Member

I wonder if we should instead just add something like deployment.virtualbox.vmFlags that would pass everything to VBoxManager modifyvm ${vmFlags}. Otherwise we'll end up with dozens of flags eventually.

@Cireo
Copy link
Contributor Author

Cireo commented Nov 3, 2016

To be honest, I think the total number of common flags could be relatively small (less than a dozen), and this offers a good way to be consistent across multiple virtualization options. I suppose this is a tradeoff between easily switching deployment providers (so you only have to learn once) and having configuration options correspond to the vendor implementation (which makes it easier if you already know your vendor).

That being said, I think a vmFlags would be a nice addition to ensure there is always some way to pass things in, and to make whatever customization options you want.

@Cireo
Copy link
Contributor Author

Cireo commented Nov 5, 2016

Should I send up another commit that augments or replaces this commit with a vmFlags option?

@domenkozar
Copy link
Member

It's not ideal to override vcpu to be 1 by default. It would be better to not have a default and only pass the number if it's set.

@domenkozar
Copy link
Member

@Cireo if you make that change I'll merge

We follow the naming convention used by libvirt, and convert this
option to a "--cpu" command line option for VBoxManage.
I am not sure about the handling of nullOr, some places seem to assume
null values will be populated as None, others seem to assume the key
will be omitted.
@Cireo
Copy link
Contributor Author

Cireo commented Jan 9, 2017

@domenkozar sorry for the delays. I have rebased against the latest nixops master, and added the options you suggested.

@domenkozar
Copy link
Member

Thanks!

@domenkozar domenkozar merged commit fc43d9c into NixOS:master Jan 9, 2017
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.

None yet

2 participants