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 #22798 Include the vmFolder in the list_vms filter spec #5297

Merged

Conversation

agrare
Copy link
Contributor

@agrare agrare commented Mar 6, 2018

If a VM is in the root of the datacenter its parent is the vmFolder.
This folder was being skipped by the PropertyFilterSpec causing an
undefined method error '[]' for nil:NilClass when accessing the
vm['parent'] key.

git bisect points to e7e9821 as introducing this issue.

Backtrace:

2018-03-06T11:11:34 fd0ae69d [app] [W] Error has occurred while listing VMs on dev-vc65 (VMware) | NoMethodError: undefined method `[]' for nil:NilClass
/home/agrare/src/foreman/app/services/fog_extensions/vsphere/mini_servers.rb:61:in `lookup_parent_folders'
/home/agrare/src/foreman/app/services/fog_extensions/vsphere/mini_servers.rb:62:in `lookup_parent_folders'
/home/agrare/src/foreman/app/services/fog_extensions/vsphere/mini_servers.rb:56:in `block in set_folder_paths'
/home/agrare/src/foreman/app/services/fog_extensions/vsphere/mini_servers.rb:55:in `each'
/home/agrare/src/foreman/app/services/fog_extensions/vsphere/mini_servers.rb:55:in `set_folder_paths'\
/home/agrare/src/foreman/app/services/fog_extensions/vsphere/mini_servers.rb:50:in `generate_folder_inventory'
/home/agrare/src/foreman/app/services/fog_extensions/vsphere/mini_servers.rb:21:in `all'

Which is failing on this line attrs['path'] = folder_inventory[vm['parent']._ref][:path] [ref] because folder_inventory doesn't include the datacenter.vmFolder.

Before fix:
screenshot from 2018-03-06 11-32-32

After fix:
screenshot from 2018-03-06 11-55-55

Fixes #22798 - VMware: Exception listing VMs in the root of the datacenter

@theforeman-bot
Copy link
Member

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

2 similar comments
@theforeman-bot
Copy link
Member

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

@theforeman-bot
Copy link
Member

Do not merge! This patch has not been tested yet.

Can an existing organization member please verify this patch?

@theforeman-bot
Copy link
Member

Issues: #22798

@theforeman-bot
Copy link
Member

There were the following issues with the commit message:

  • c17a53d must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@theforeman-bot
Copy link
Member

There were the following issues with the commit message:

  • ef566db must be in the format fixes #redmine_number - brief description

If you don't have a ticket number, please create an issue in Redmine.

More guidelines are available in Coding Standards or on the Foreman wiki.


This message was auto-generated by Foreman's prprocessor

@agrare
Copy link
Contributor Author

agrare commented Mar 7, 2018

Hey @dLobatog not sure if you're the right person to review this but you looked at #5171, could you take a look at this?

@ohadlevy
Copy link
Member

@agrare welcome!
@timogoebel mind having a look?

@ohadlevy
Copy link
Member

@agrare in your screenshots, I can see that folder has no value? I assume it should be / ?

@timogoebel
Copy link
Member

@agrare: Thanks for the fix and welcome. Looks good to me. Can you please also take a look at @ohadlevy's comment?

If a VM is in the root of the datacenter its parent is the vmFolder.
This folder was being skipped by the PropertyFilterSpec causing an
undefined method error '[]' for nil:NilClass when accessing the
vm['parent'] key.
@agrare
Copy link
Contributor Author

agrare commented Mar 12, 2018

I can see that folder has no value? I assume it should be / ?

Before this change VMs in the root of the datacenter would have a blank folder property.

It looks like e7e9821 also broke the MiniServer initializer by passing attrs['path'] instead of attrs[:path] which was causing no folders to be shown, the most recent commit fixes this and this is the new screenshot

screenshot from 2018-03-12 09-21-08

@ohadlevy
Copy link
Member

[ok to test]

@timogoebel
Copy link
Member

timogoebel commented Mar 12, 2018

@agrare: Thanks. I am testing this right now, but only get an empty vm list. I'll try to look into it further.

Copy link
Member

@timogoebel timogoebel left a comment

Choose a reason for hiding this comment

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

I take my last comment back, works very well. Thanks @agrare. Will merge pending Jenkins.

@agrare
Copy link
Contributor Author

agrare commented Mar 12, 2018

Thanks for the review @timogoebel !

@ohadlevy
Copy link
Member

[test foreman]

@ohadlevy ohadlevy merged commit 68e96c1 into theforeman:develop Mar 13, 2018
@ohadlevy
Copy link
Member

thanks @agrare!

@agrare agrare deleted the 22798_fix_list_vms_in_root_of_datacenter branch March 13, 2018 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants