Drop supports_feature? methods

[kbrock]

Do not merge until we talk

Overview

Change implementation of supports to not define methods.

1. https://github.com/ManageIQ/manageiq/issues/21989
2. https://github.com/ManageIQ/manageiq/issues/21179
3. https://github.com/ManageIQ/manageiq/pull/21553 - avoiding &:supports_feature? calls

Before

1. supports :feature and supports_not :feature defines supports_{feature}?.
2. if the feature is not supported, supports_{feature}? calls add_unsupported_reason to store the reason.
3. supports?(feature) calls supports_{feature}? under the covers.
4. supports?(unknown_feature) detects when supports_{unknown_feature}? is not defined and directly calls add_unsupported_reason.

After

1. supports :feature and supports_not :feature stores the reasons in a class level hash.
2. if the feature is not supported, supports_{feature}? calls add_unsupported_reason to store the reason.
3. supports?(feature) uses class level hash to determine the reason. It leverages add_unsupported_reason the same as before.
4. supports?(unknown_feature) notices that the feature is not in the class level hash and directly calls add_unsupported_reason.

Highlights

Fix infinite stack trace for Supports Attribute

We have a feature called supports_attributes that introduces a virtual attribute supports_{feature} (note: no trailing question mark). Since this is a boolean attribute, Rails defines this method with a question mark, which conflicts with the method that supports :feature defines. (see before number 1)

So when the code tries to determine if the feature is defined (see before number 4), it sees the rails attribute definition and assumes it is always defined and calls the rails attribute accessors.

Rails 6.1 worked fine with this bug, but in rails 7, it causes an infinite stack trace. This PR removes this duplicate definition issue and fixes both rails 6.1 and 7.0

Dropping supports_feature? methods

The supports_{feature}? method is useful when using detect or select -- detect(&:supports_feature?), so we used it a few times in the ui-classic and in core. We have pruned these out but unfortunately we do loose this shortcut. I think subclasses_supporting has made those calls no longer necessary.

Determining if supports_feature? methods are used

Searching the code base for calls to supports_feature? is a little tricky since there are methods with the name supports_feature? defined, but they are not using the supports :feature mechanism. There is an initiative (linked at the top) to converge on the supports :feature mechanism, but that has been a background thread

Reducing the number of methods defined

I remember having a conversation with @chessbyte to cut down on the number of methods defined in classes, but I can't find an issue actually stating this goal. That was the main reason I had embarked on this journey some dozen PRs ago.

We have not completed to goal to converge all the supports type mechanisms. But this PR completes the goal to reduce all the extra methods defined on the class.

We did reduce quite a few methods defined by #22578 - that no longer defines the methods for every base class. That PR definitely exposed the problem we are seeing now, but supports should not be going through the rails attributes code, so that is why I am suggesting this solution

[kbrock]

update:

• changed supports_order alias (a wip)
• fixed rubocops for hashrockets, extra space
• dropping unused COMMON_FEATURE constant and present? => presence

ok, supports_order? is being used, though I think it was introduced in https://github.com/ManageIQ/manageiq/pull/19186 / https://github.com/ManageIQ/manageiq-api/pull/656

I'll see if that is used anywhere. Since these sometimes get exposed as columns,

WIP: possibly drop supports_order / validate_supports WIP: want to discuss before merging (and run cross tests

[jrafanie]

FYI, I tried this PR with my rails 7 branch and things look good.

[Fryguy]

The only thing I can think of that I didn't see covered in any of the comments here (unless I missed it), is that we have a handful of virtual columns representing "supports", which are meant to be accessed over the API. As long as those still work, I'm good with this.

[kbrock]

@Fryguy

Thanks I changed Highlights / Supports Attribute to better highlight attribute_supports. These supports_attributes are what caused this issue in the first place.

[kbrock]

update:

• dropped aliases to orderable? (PR in ui-classic to drop)
• dropped validate - not able to find this anywhere

update:

• no longer delete validate_order - looks like it may be called by order_#{action} from ui-classic

[kbrock]

update:

• removed the deletion of ServiceTemplate#orderable? (OrchestrationTemplate implements this and this is no longer a quick change - we can do this work in a separate PR if we even want to do this)

[kbrock]

update:

• simplified default_supports_reason
• no longer setting a default reason keeping false value. that way it can be localized later
• added specs that chain supports. This is a code pattern that we have been using more and more. These ensure the pattern works as designed.

[kbrock]

update:

• rebase (no conflicts)
• merged check_supports into class level supports?

update:

• pulled check_supports back out of supports?. I had started to make this private and enhance, but in the end of the day, this method is going away once we merge #22898

[kbrock]

update:

• rebased (resolved conflict - supports?(:order) fix was fixed by #22902)

[kbrock]

WIP: I found some supports_feature? methods in a few providers

[kbrock]

searching for supports_:

docs to invalid methods (doesn't affect build):

• [ ] supports_cockpit (docs)
• [ ] supports_console (docs) -- need to expose?
• [ ] supports_vnc_console (docs)
• [ ] javascript grep decorators=vms.supports_*?

ignore:

• tools supports_nothing?
• inventory_refresh supports_building_inventory_collection?

currently defined: (and handled in part of https://github.com/ManageIQ/manageiq/issues/21989 )

• supports_sti?
• supports_pxe?
• supports_iso?
• supports_cloud_init?
• supports_sysprep?
• supports_customization_template?
• supports_systemd?
• supports_nohup_and_backgrounding
• supports_instance_store
• supports_ebs
• supports_security_groups ?
• supports_button_action
• supports_native_clone
• supports_limit

columns:

• supports_32_bit?
• supports_64_bit?
• supports_hvm?
• supports_paravirtual?

methods with attributes

• supports_column?(v)
• supports_remote_data_timestamp?(v)
• supports_remote_data_version?(v)
• supports_resource_version?(v)
• supports_authentication(v)
• supports_command
• supports_migrate_for_all

[kbrock]

un-WIP - there should be no supports_feature? methods defined

[kbrock]

@miq-bot cross_repo_tests #22883 /all

[kbrock]

cross repo test failures:

• [x] automate (waiting for merge)
• [x] api
• [x] kubernetes (waiting for merge)
• [ ] ui-service

[kbrock]

update:

• rebased (to pull in agrare's change around managers - to turn kubernetes green)

[kbrock]

@Fryguy You have any concerns dropping the supports_feature? syntax from our code base?

[Fryguy]

Nope I love it

[kbrock]

update:

• rebase (fix merge conflicts)
• moved commit order a little

[miq-bot]

Checked commits https://github.com/kbrock/manageiq/compare/15330bfe5d628e6def013dd387bfd0dd496a7810~...181a03e72098871c1ac55fdc279ee78e32ff1e4f with ruby 2.7.8, rubocop 1.56.3, haml-lint 0.51.0, and yamllint 5 files checked, 0 offenses detected Everything looks fine. :cookie: