search

ManageIQ / manageiq

ManageIQ Open-Source Management Platform
https://manageiq.org
Apache License 2.0
1.35k stars 900 forks source link

Promote supports feature to ApplicationRecord #23140

Open kbrock opened 3 months ago

kbrock commented 3 months ago

Most models include SupportsFeatureMixin. This moves up to ApplicationRecord +1/-73

Fryguy commented 3 months ago

@agrare Please also review.

kbrock commented 3 months ago

update:

miq-bot commented 3 months ago

Checked commit https://github.com/kbrock/manageiq/commit/bfbecf97d8d71c49e96f2b503951364bce5ca422 with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint 64 files checked, 0 offenses detected Everything looks fine. :trophy:

Fryguy commented 2 months ago

Most models include SupportsFeatureMixin. This moves up to ApplicationRecord +1/-73

I understand the intent to avoid duplication here, but "most models" isn't really accurate. There are 457 models roughly, and only 73 have this, which means only 16% of models are like this.

One upside to having it in specific models is we can know what models are the things that are "CI"s (i.e. things that we externally manage). That gets me wondering if we should have a CIMixin where we take a lot of these common mixins across CIs, then include that. But overall, I'm not sure that would make any benefit.

I'm not against this PR, but I'm not sure - would like a second opinion from @agrare and/or @jrafanie

jrafanie commented 2 months ago

One upside to having it in specific models is we can know what models are the things that are "CI"s (i.e. things that we externally manage). That gets me wondering if we should have a CIMixin where we take a lot of these common mixins across CIs, then include that. But overall, I'm not sure that would make any benefit.

haha, if you can name what this commonality is, we could probably create it. Ideally, all of these things should be reportable, support features, taggable, etc. We can override in classes that can't do one or more of these things if desired. Not sure how much we want to pull into the common place.

kbrock commented 2 months ago

I think having a plugin that says "I'm a taggable, rbac applyable, data model that is in each provider" may have merit.