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

Fixes #30552 - Fix vmware vm cloning when volumes are not present #7942

Merged
merged 1 commit into from Nov 16, 2020

Conversation

yifatmakias
Copy link
Contributor

@yifatmakias yifatmakias commented Aug 30, 2020

No description provided.

@theforeman-bot
Copy link
Member

Issues: #30552

Comment on lines +551 to +552
"datastore" => args[:volumes].empty? ? nil : args[:volumes].first[:datastore],
"storage_pod" => args[:volumes].empty? ? nil : args[:volumes].first[:storage_pod],
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look like it's specific to hammer, would a clearer commit message be something like "fix vmware vm cloning when volumes are not present"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I agree I will change the commit message.

@yifatmakias yifatmakias changed the title Fixes #30552 - Fix undefined error when creating vmware host in hammer Fixes #30552 - Fix vmware vm cloning when volumes are not present Sep 1, 2020
@yifatmakias
Copy link
Contributor Author

[test katello]

@yifatmakias
Copy link
Contributor Author

[test katello]

Copy link
Contributor

@shiramax shiramax left a comment

Choose a reason for hiding this comment

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

@yifatmakias looks good!
can you add tests?

},
},
"provision_method" => "image"
)

Choose a reason for hiding this comment

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

Layout/ClosingParenthesisIndentation: Indent ) to column 6 (not 4)

"_delete" => "",
},
"0" => {
"type" => "VirtualVmxnet3",

Choose a reason for hiding this comment

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

Layout/FirstHashElementIndentation: Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.

"cpus" => "1",
"interfaces_attributes" => {
"new_interfaces" => {
"type" => "VirtualE1000",

Choose a reason for hiding this comment

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

Layout/FirstHashElementIndentation: Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.

"image_id" => "2",
"cpus" => "1",
"interfaces_attributes" => {
"new_interfaces" => {

Choose a reason for hiding this comment

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

Layout/FirstHashElementIndentation: Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.

},
)
attrs_parsed = HashWithIndifferentAccess.new(
"image_id" => "2",

Choose a reason for hiding this comment

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

Layout/FirstArgumentIndentation: Indent the first argument one step more than the start of the previous line.

"network" => "network-17",
"_delete" => "",
},
},

Choose a reason for hiding this comment

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

Style/TrailingCommaInArguments: Avoid comma after the last parameter of a method call.

"_delete" => "",
},
"0" => {
"type" => "VirtualVmxnet3",

Choose a reason for hiding this comment

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

Layout/FirstHashElementIndentation: Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.

"cpus" => "1",
"interfaces_attributes" => {
"new_interfaces" => {
"type" => "VirtualE1000",

Choose a reason for hiding this comment

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

Layout/FirstHashElementIndentation: Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.

"image_id" => "2",
"cpus" => "1",
"interfaces_attributes" => {
"new_interfaces" => {

Choose a reason for hiding this comment

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

Layout/FirstHashElementIndentation: Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.

@@ -218,6 +218,49 @@ class Foreman::Model::VmwareTest < ActiveSupport::TestCase
assert_equal mock_vm, cr.create_vm(attrs_in)
end

test "#clone_vm works with no volume attributes" do
attrs_in = HashWithIndifferentAccess.new(
"image_id" => "2",

Choose a reason for hiding this comment

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

Layout/FirstArgumentIndentation: Indent the first argument one step more than the start of the previous line.

@yifatmakias
Copy link
Contributor Author

@shiramax Added tests let me know if it is ok :)

@shiramax
Copy link
Contributor

@yifatmakias code looks good,
however, I'm not sure that the best way here is to pick the first volume. maybe if the volume didn't choose we should tell the user he must choose?
Also, in the Redmine issue, I don't see that the bug is happening in VM clone but in host creation.

@shiramax
Copy link
Contributor

@yifatmakias we talked about this PR offline. good look good, merging this one.

@shiramax shiramax merged commit 62ac9fe into theforeman:develop Nov 16, 2020
@tbrisker
Copy link
Member

2.3 - fbbc297

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants