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

memory parameter should be false if missing #3707

Merged
merged 3 commits into from
Aug 10, 2015

Conversation

mkanoor
Copy link
Contributor

@mkanoor mkanoor commented Aug 3, 2015

The VMWare snapshot API takes 3 parameters name, description and memory.
memory cannot be nil if not specified, it should be false for the VIM api to work properly.
The Automate Service model now exposes the memory parameter and defaults it to false if not
specified.

https://bugzilla.redhat.com/show_bug.cgi?id=1247664

@mkanoor
Copy link
Contributor Author

mkanoor commented Aug 3, 2015

@gmcculloug @Fryguy
Please review

@@ -393,7 +393,7 @@ def self.task_arguments(options)
when "remove_snapshot", "revert_to_snapshot" then
[options[:snap_selected]]
when "create_snapshot" then
[options[:name], options[:description], options[:memory]]
[options[:name], options[:description], options.fetch(:memory, false)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this issue is specific to VMware, why is the solution to modify base class VmOrTemplate?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only problem with this (which I guess is my problem with .fetch in general), is that it only handles keys that don't exist.

{}.fetch(:a, false)
# => false

{:a => nil}.fetch(:a, false)
# => nil

To be safe, this might be better written as !!options[:memory] if all you want is a literal boolean.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chessbyte @Fryguy

on further discussions with Greg M we agreed to expose the memory parameter via the service model apis for vmware. This way the method api will enforce that the memory value can only be true|false.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach fixes the original issue and adds support for passing the memory flag from automate. Also we want to move the snapshot methods out of the base automate service model and into the VMware sub-classed model where it belongs since these methods are currently not supported on other providers.

mkanoor added 2 commits August 5, 2015 13:58
Sets the default memory value to be false unless specified
by the automate method.
Added specs to validate the snapshot parameters

https://bugzilla.redhat.com/show_bug.cgi?id=1247664
@miq-bot
Copy link
Member

miq-bot commented Aug 5, 2015

Checked commits mkanoor@978c112 .. mkanoor@6dac115 with rubocop 0.32.1 and haml-lint 0.13.0
2 files checked, 1 offense detected

lib/miq_automation_engine/service_models/miq_ae_service_manageiq-providers-vmware-infra_manager-vm.rb

@mkanoor
Copy link
Contributor Author

mkanoor commented Aug 7, 2015

@gmcculloug @Fryguy @chessbyte
Please review. We have now exposed the memory attribute to automate via the service models.

@mkanoor
Copy link
Contributor Author

mkanoor commented Aug 10, 2015

@Fryguy @chessbyte @gmcculloug
Please review

Fryguy added a commit that referenced this pull request Aug 10, 2015
memory parameter should be false if missing
@Fryguy Fryguy merged commit c4b7974 into ManageIQ:master Aug 10, 2015
@Fryguy Fryguy added this to the Sprint 28 Ending August 24, 2015 milestone Aug 10, 2015
lfu added a commit to lfu/manageiq that referenced this pull request Nov 21, 2016
The snapshot code in automate was moved from VmOrTemplate to VMware specific class by PR ManageIQ#3707 when VMware was the only class that supports snapshot.
But snapshot support has been added to RHEVM and Openstack as of version Euwe.
The snapshot code in automate should be available at Vm level.

https://bugzilla.redhat.com/show_bug.cgi?id=1395175
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.

5 participants