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

ExtManagementSystem - use supports_not :admin_ui; use supports_not #16532

Closed
wants to merge 2 commits into from
Closed

ExtManagementSystem - use supports_not :admin_ui; use supports_not #16532

wants to merge 2 commits into from

Conversation

himdel
Copy link
Contributor

@himdel himdel commented Nov 27, 2017

Displaying a toolbar button to open the admin ui tries to call the supports method, and when not there, falls back to the old availability code, which fails with missing validate_admin_ui method.

This is because since ManageIQ/manageiq-providers-ovirt#133, RedHat::InfraManager supports :admin_ui, but the supports_not call was not added to the ancestor class.

Adding, and taking the opportunity to use supports_not for all the def supports_foo? ; false ; end methods.

Cc @borod108 , @vojtechszocs , @martinpovolny

Displaying a toolbar button to open the admin ui tries to call the supports method, and when not there, falls back to the old availability code, which fails with missing `validate_admin_ui` method.

This is because since ManageIQ/manageiq-providers-ovirt#133, `RedHat::InfraManager` supports :admin_ui, but the `supports_not` call was not added to the ancestor class.

Adding, and taking the opportunity to use `supports_not` for all the `def supports_foo? ; false ; end` methods.
@himdel
Copy link
Contributor Author

himdel commented Nov 27, 2017

@miq-bot add_label bug, gaprindashvili/yes

@himdel
Copy link
Contributor Author

himdel commented Nov 27, 2017

from gitter November 27, 2017 2:35 PM

[----] F, [2017-11-27T16:21:44.496001 #9015:4469e80] FATAL -- : Error caught: [ActionView::Template::Error] undefined method validate_admin_ui' for #<ManageIQ::Providers::Redhat::InfraManager:0x007fa7876574b8>
Did you mean? validates_numericality_of
/home/bod108/.rvm/gems/ruby-2.4.1/gems/activemodel-5.0.6/lib/active_model/attribute_methods.rb:433:inmethod_missing'
/home/bod108/git/manageiq/app/models/mixins/availability_mixin.rb:24:in is_available?' 
/home/bod108/git/manageiq/plugins/manageiq-ui-classic/app/helpers/application_helper/button/generic_feature_button.rb:13:inrescue in visible?'
/home/bod108/git/manageiq/plugins/manageiq-ui-classic/app/helpers/application_helper/button/generic_feature_button.rb:10:in `visible?'

Copy link

@borod108 borod108 left a comment

Choose a reason for hiding this comment

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

the tests seem to fail

@himdel
Copy link
Contributor Author

himdel commented Nov 27, 2017

:api_version is not in the mixin's list - undoing that change - none of them are in the list

@himdel himdel closed this Nov 27, 2017
@himdel himdel deleted the fix-supports-admin-ui branch November 27, 2017 15:20
@himdel
Copy link
Contributor Author

himdel commented Nov 27, 2017

Never mind, SupportsFeatureMixin does call supports_not for each feature.

The problem happens when that method fails when calling another method - like NoMethodError: undefined method 'version_higher_than?'

@miq-bot
Copy link
Member

miq-bot commented Nov 27, 2017

Checked commits https://github.com/himdel/manageiq/compare/70d40a1c9a7a3d058d009a9a6d82a362ebce882c~...305829b97fb83c8999bed1864104c73b21aea55f with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

@masayag
Copy link
Contributor

masayag commented Nov 27, 2017

@himdel thanks - should be fixed by ManageIQ/manageiq-providers-ovirt#156

@vojtechszocs
Copy link
Contributor

This is because since ManageIQ/manageiq-providers-ovirt#133, RedHat::InfraManager supports :admin_ui, but the supports_not call was not added to the ancestor class.

While working on #16403 I've encountered some errors (don't remember the details, sorry) after adding supports_not :admin_ui to app/models/ext_management_system.rb so I solved that by updating app/models/mixins/supports_feature_mixin.rb (see here).

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

Successfully merging this pull request may close these issues.

5 participants