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

[Providers] Pluggability Checklist #19440

Open
24 of 48 tasks
agrare opened this issue Oct 28, 2019 · 15 comments
Open
24 of 48 tasks

[Providers] Pluggability Checklist #19440

agrare opened this issue Oct 28, 2019 · 15 comments

Comments

@agrare
Copy link
Member

agrare commented Oct 28, 2019

This is an ongoing list of items which need to be extracted out of core repos (manageiq, manageiq-api, manageiq-ui) and into plugins.

A continuation of #14840

Specs

Providers

Core Pluggability

UI

@agrare
Copy link
Member Author

agrare commented Oct 28, 2019

cc @Fryguy @chessbyte

@agrare agrare mentioned this issue Oct 28, 2019
38 tasks
@juliancheal
Copy link
Member

juliancheal commented Oct 29, 2019

One thing @agrare is removing the need to check node_types (in core, I guess) i.e. "nodes" (openstack) or "hosts" (non-openstack) for instance here https://github.com/ManageIQ/manageiq/blob/master/app/models/host.rb#L1722-L1744

@agrare
Copy link
Member Author

agrare commented Oct 30, 2019

That's a good one @juliancheal, we can probably just drop the title_for_host and title_for_clusters methods in the UI then those methods aren't needed at all

@agrare agrare self-assigned this Oct 30, 2019
@Fryguy
Copy link
Member

Fryguy commented Oct 31, 2019

I had spoken to @chessbyte about that with respect to his EmsCluster changes (see here), and we came up with 2 possibilities

  • Drop the "feature" entirely
  • If we really need the feature, then one way to do it pluggably is to make the display_name method on the class deal with it. That is, the base display_name method would query all of the known types, then ask each of the known types what their preferred_display_name is, and then the base method just combines them together with .join(" / "). Then the plugin owns the name as well as the i18n for it. Then, in the ui code, this, this, and this effectively go away.

@chessbyte
Copy link
Member

Created #19469 for refactoring VmScan code.

@carbonin
Copy link
Member

@agrare Does #19536 solve the worker definitions bullet or is that more complicated?

@agrare
Copy link
Member Author

agrare commented Nov 20, 2019

@carbonin that absolutely covers that case

@NickLaMuro
Copy link
Member

@agrare This exists, which I am currently working on:

ManageIQ/manageiq-gems-pending#403

Not sure where it fits into what you have at the moment. Sorry it took me so long to figure the git-stuff out for that...

@agrare
Copy link
Member Author

agrare commented Nov 25, 2019

Awesome thanks @NickLaMuro, added to the list and linked to your issue.

@NickLaMuro
Copy link
Member

For what it is worth, for the end goal of that ticket item:

Move provider specifics from MiqFileStorage into their respective provider repos e.g. miq_s3_storage.rb/miq_swift_storage.rb (ManageIQ/manageiq-gems-pending#403) @NickLaMuro

That PR doesn't complete that, but just moves it back into the main repo via:

#19547

Moving the swift and s3 parts to their respective provide repos is a different step all together, and figuring out how we make those "pluggable" is something I haven't quite figured out yet, and might require a bit of discussion. I might try tackling that once the two above are merged though.

@Fryguy
Copy link
Member

Fryguy commented Nov 26, 2019

Moving the swift and s3 parts to their respective provide repos is a different step all together, and figuring out how we make those "pluggable" is something I haven't quite figured out yet, and might require a bit of discussion. I might try tackling that once the two above are merged though.

This is one of those weird cases where even though it's S3, it's been argued that it doesn't belong in the amazon provider. The reasoning is that someone could use S3 as a File Depot, without requiring an Amazon provider and vice versa. This is sort of an independent pluggable thing. I didn't think this way previously, but now I'm leaning more towards that way.

@skateman
Copy link
Member

skateman commented Mar 13, 2020

I got a green light from @Fryguy to move decorators, we might introduce external tests from the decorator library repo for providers using a similar approach to https://github.com/ManageIQ/manageiq/blob/master/lib/tasks/test_providers_common.rake
https://github.com/ManageIQ/manageiq/blob/master/spec/models/event_stream_spec.rb

@Fryguy
Copy link
Member

Fryguy commented Mar 13, 2020

I added a "Decorators" bullet to the checklist in the OP. @skateman Be sure that if there are any patterned changes that all providers need to implement, that they are first implemented in the provider generator and then blasted out to all existing providers.

Fryguy added a commit to Fryguy/manageiq that referenced this issue Aug 26, 2020
Fryguy added a commit to Fryguy/manageiq that referenced this issue Aug 26, 2020
agrare added a commit to agrare/manageiq that referenced this issue Sep 24, 2020
Update the spec test to not require a hard-coded exhaustive list of
EMS types from all of the plugins

ManageIQ#19440
@Fryguy
Copy link
Member

Fryguy commented Sep 24, 2020

Merged #20595, but wasn't sure which checklist item above to check off

@agrare
Copy link
Member Author

agrare commented Sep 24, 2020

@Fryguy linked the pr and checked it off

@Fryguy Fryguy added this to Roadmap Jun 12, 2024
@Fryguy Fryguy moved this to Backlog in Roadmap Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog
Development

No branches or pull requests

7 participants