API - VMs collection - replace decorators with SupportsFeatureMixin

[himdel]

This removes the only remaining use of decorators from manageiq (not ui).

Service UI is using ?decorators=vms.supports_console?,vms.supports_cockpit?, so I suppose we need to preserve that. - Implementing only those two, by calling vm.supports_*?.

Since console support is not using the SupportsFeatureMixin, added it to VmOrTemplate, using the same code that was previously in VmOrTemplateDecorator.

Cockpit support is using SupportsFeatureMixin, but uses a different name than the decorator (supports_cockpit? vs supports_launch_cockpit?).

(Related to (but not dependent upon) ManageIQ/manageiq-ui-classic#237.)

Testing: (with a valid service ID, which has multiple vms..) GET /api/services/10000000000245?expand=vms&decorators=vms.supports_console%3F%2Cvms.supports_cockpit%3F

Closes ManageIQ/manageiq-ui-classic#3

@abellotti, @jntullo WDYT? :)

[abellotti]

may end up with lotsa churn with this in API every-time there's a new decorator. Would prefer logic to be driven by "support_" prefix and calling the appropriate method on vm. Not sure about the differently named _cockpit? vs. launch_cockpit?, maybe define an alias in the model for compatibility so this code here is generic for all. Thanks.

[himdel]

@abellotti My impression was that people want decorators gone from the API .. that's why I took this path, we still need to support the ones that were used, but the idea of this PR is that those would be the only ones. (And can be replaced/deprecated in a future API version.)

Does your objection still stand given that?

[abellotti]

Ahh ok, so keeping this for compatibility 🙇 new and future code would then leverage the attributes= instead. Does the following work today then ?

GET /api/vms?expand=resources&attributes=supports_launch_cockpit?,supports_console?

[himdel]

Exactly :). The attributes version does not work today, because SupportsFeatureMixin does not create virtual attributes.

Not sure if the way forward is to make SupportFeatureMixin create virtual attributes for every feature, or to add special support for these in attributes, or to introduce a new query parameter..

[abellotti]

we can probably extend app/controllers/api/base_controller/renderer.rb's attr_virtual?/attr_accessible? to "support" those so they come for free with attributes= without having to change the query parameters.

[himdel]

@abellotti Sounds good, do you want me to do it in this PR, or would you rather have your team do it..?

[Fryguy]

I'm wondering if we should have capabilities or something on the API objects...so not necessarily attributes, but implemented similarly

[abellotti]

while we certainly can add that on the server side, I just don't know how they'd be specified in the API client. I'm hoping we can still somehow use .select(...) for those and won't need something like search_options(...) as implemented in ManageIQ/manageiq-api-client#63 for collection_class.

[miq-bot]

This pull request is not mergeable. Please rebase and repush.

[himdel]

@abellotti is this blocked by anything?

[miq-bot]

Checked commits https://github.com/himdel/manageiq/compare/4dcd311c92e69946f94548c8f2813f60d464baee~...21934bfeaf5127683ecc9386c7b0331cefef9537 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 1 offense detected

app/models/mixins/supports_feature_mixin.rb

• ❗ - Line 78, Col 5 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.

[abellotti]

I don't think so, just me. we can introduce the capabilities/supports in a separate PR.

Thanks @himdel for fixing this. 👍