adds flag to config to expunge vm immediately on destroy #75
Conversation
| @@ -417,6 +422,9 @@ def finalize! | |||
| # private ip is nil by default | |||
| @private_ip_address = nil if @private_ip_address == UNSET_VALUE | |||
|
|
|||
| # expunge on destroy is nil by default | |||
| @expunge_on_destroy = nil if @expunge_on_destroy == UNSET_VALUE | |||
There was a problem hiding this comment.
Wouldn't is be better to make @expunge_on_destroy = false by default?
I understand that making it nil will have the same effect because later you do ... if expunge_on_destroy != nil (at L105 of terminate_instance https://github.com/schubergphilis/vagrant-cloudstack/pull/75/files#diff-b45563ef209cc71636c6684e13c4a82eR105) , but if you never set it to nil, you don't actually need the extra check, and can always pass the value to the API call.
This will probably have impact on the spec tests too.
There was a problem hiding this comment.
I thought about that. But I'm not completely clear on what will happen if I pass this option to a cloudstack API that doesn't support the expunge option. Will it silently ignore adding extra parameters to the call?
There was a problem hiding this comment.
CloudStack will ignore the extra parameters, but if the API call does not support it and a user still defines the parameter, then it will be passed to the API call and ignored. All this while the user expects the behaviour.
You are passing this to the server.destroy and we have to make sure this one accepts the parameter, right?
There was a problem hiding this comment.
It would be nice if we could chek if this parameter does anything. But fog passes anything we add to options directly to cloudstack api
expunge_on_destroy default to false
expunge on destroy default to false
|
That's it. I will merge this as soon as I can test it (the cloud guys are working on fixing something first). One comment: the commit messages you use are not that informative of what you did. Food for thought. |
|
I admit I'm struggling with that. Maybe I should try reading other people's commits to learn from :-) |
|
sorry wrong button |
|
@j00p34 one test is failing! |
fix test. expunge_on_destroy false
|
test fixed |
|
I merged other PR before yours and because of that it does not merge cleanly anymore. Sorry for that. |
|
Very busy at the moment, so if it's not to much trouble.. else give me a day and I'll fix it myself |
|
Merged manually in commit 3320126 |
FIXES #74