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 #28471 - Fixed invalid cluster error-vmware host creation #7239

Closed
wants to merge 1 commit into from

Conversation

yifatmakias
Copy link
Contributor

…host

@theforeman-bot
Copy link
Member

Issues: #28471

@yifatmakias yifatmakias changed the title Fixes #28471 - Fixed error of invalid cluster when creating a vmware … Fixes #28471 - Fixed invalid cluster error-vmware host creation Dec 10, 2019
Copy link
Member

@ezr-ondrej ezr-ondrej left a comment

Choose a reason for hiding this comment

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

I would prefer this check in parse_args method. Also could we just check !args['cluster'].blank?.
Your implementation make sense, but it makes one more request to the vmware (see failing tests), what's not the best and I would prefer to avoid it if possible and we are not checking the other args for existence, so I think it's ok if we won't check cluster eighter.

@yifatmakias
Copy link
Contributor Author

It's just that when I tried to create a VMware host I accidentally forgot to enter the cluster in the host group I created and a very non-indicative error appeared: Failed to create a compute vmware (VMware) instance jenny-scites.redhat.tlv.com: undefined method `resourcePool' for #RbVmomi::VIM::Folder:0x00007f8ecb26c418.

So I thought it will be better to throw a more informative error. Do you think that in order to avoid one more request to the VMware it is better to leave it as it is today?

@ezr-ondrej
Copy link
Member

It's just that when I tried to create a VMware host I accidentally forgot to enter the cluster in the host group I created and a very non-indicative error appeared: Failed to create a compute vmware (VMware) instance jenny-scites.redhat.tlv.com: undefined method `resourcePool' for #RbVmomi::VIM::Folder:0x00007f8ecb26c418.

So I thought it will be better to throw a more informative error. Do you think that in order to avoid one more request to the VMware it is better to leave it as it is today?

There is many weird errors like that happening throught the compute resources. Maybe we can solve it by adding such check on the fog side, so the error returned from there would make more sense?

Don't get me wrong, I would love to check params passed to compute resources on our side, but we should do it for all the params in some proper way.

@ezr-ondrej
Copy link
Member

@yifatmakias do you thing this: fog/fog-vsphere#240 would be proper solution? :)
We do the same for cloning the machine (image provisioning), so it make sense to me.

@yifatmakias
Copy link
Contributor Author

@ezr-ondrej I agree this is a better solution.
I will close this pull request.
Thanks!

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