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

Raise error in case of booting from volume and from image #81

Merged
merged 1 commit into from
Apr 23, 2019

Conversation

shiramax
Copy link
Contributor

@shiramax shiramax commented Apr 23, 2019

@@ -150,6 +150,13 @@ def create_vm(args = {})
volume.type = 'containerDisk'
volumes << volume
end
is_bootable = false
volumes_attributes.each do |_, v|
Copy link
Contributor

Choose a reason for hiding this comment

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

you can simplified that to:
raise ::Foreman::Exception.new(N_('It is not possible to set a bootable volume and image based provisioning.')) if args.key?("image_id") && volumes_attributes.any? { |_,v| v["bootable"] == "true" }

Copy link
Contributor

Choose a reason for hiding this comment

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

also, you can pull this out to a method and add a test to cover it.

@shiramax
Copy link
Contributor Author

@masayag done, please review again

@@ -150,6 +156,8 @@ def create_vm(args = {})
volume.type = 'containerDisk'
volumes << volume
end
verify_booting_from_image_is_possible(args)
Copy link
Contributor

Choose a reason for hiding this comment

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

If we'll move the validation to the image_provision block (line 153) , we can remove the check for the existence image_id key.

what do you think ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@masayag , done :)

@@ -142,6 +148,7 @@ def create_vm(args = {})
# Add image as volume to the virtual machine
image_provision = args["provision_method"] == "image"
if image_provision
verify_booting_from_image_is_possible(args)
Copy link
Contributor

Choose a reason for hiding this comment

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

so if we don't anything else from args, except of volumes_attributes, we can pass to this method volumes_attributes that was set to a variable on line 145.

@shiramax
Copy link
Contributor Author

@masayag done.

@masayag masayag merged commit ae5ee26 into theforeman:master Apr 23, 2019
@masayag masayag mentioned this pull request Apr 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants