-
Notifications
You must be signed in to change notification settings - Fork 145
Add ability to modify VM's configuration (number of CPUs and memory size) #16
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
Conversation
fkorotkov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was mostly wondering about arguments for run command to specify cpu/memory so it's easy experiment with them on demand.
Plus it will play nicely for persistent workers. No need to clone-set-run, can just clone and run right away.
Do you think it makes sense to include these changes here or in a separate PR?
Sources/tart/VMConfig.swift
Outdated
|
|
||
| self.memorySize = try container.decode(UInt64.self, forKey: .memorySize) | ||
|
|
||
| // Migrate to newer version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before we release it to the public we can probably ignore it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed in a8b46ba.
Honestly, I've started working on this PR with this mindset and realized that it's not a good idea in terms of transparency. Here's why: we already store VM's configuration in a file. Starting a VM with configuration other than the one described in it's configuration file is confusing in the event the user chooses to inspect the configuration file (#1 (comment)) and infer some knowledge from it. It will be misleading.
This process is fully automated, so no issue here at all.
I'd vote for a separate PR. |
SGTM. There is already #4 for that actually. |
|
On a second though why do we store |
See cirruslabs/cirrus-cli#496.