-
Notifications
You must be signed in to change notification settings - Fork 33
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
add memory_dynamic_min,memory_dynamic_max #306
Conversation
Hi @aslak11, thanks for your interest in contributing to the project! For all terraform changes, we require adding terraform acceptance tests in order to verify that the functionality works. Ensuring VM resource arguments/attributes are created and modified properly can be quite complex and sometimes requires rebooting the VM first. Given the complexity of the memory settings, having test coverage is a must and I recommend that you check out the existing VM memory tests and the contributing doc for more details. Another thing to consider since the memory support is there but half baked. We may need to perform a terraform state upgrade. I haven't thought through the static/dynamic settings yet, but we want to minimize the pain in upgrading after the dynamic settings are supported. An example of that can be found in #271. I'm happy to help you through the development of these tests and the state migration functionality. Please let me know if you have any questions or comments. |
* change vm set/create params memoryMax to memoryStaticMax * add state migrations
@aslak11 I still haven't had a chance to review this, but we need to track down the following test failures. This output was collected from the Jenkins job that ran against #316. The
|
Since there hasn't been activity here I'm going to close this pull request. I'm happy to assist if/when you are able to consider the details and test failures mentioned above. |
fixes #211