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

[WIP] Extract NetworkManager parent_manager to a mixin #20229

Closed

Conversation

agrare
Copy link
Member

@agrare agrare commented Jun 1, 2020

@agrare agrare requested review from Fryguy and kbrock as code owners June 1, 2020 13:16
@agrare agrare changed the title Extract NetworkManager parent_manager to a mixin [WIP] Extract NetworkManager parent_manager to a mixin Jun 1, 2020
@miq-bot miq-bot added the wip label Jun 1, 2020
@agrare agrare force-pushed the allow_for_top_level_network_managers branch from 9f3214c to 1c3ee1a Compare June 1, 2020 13:38
@chessbyte chessbyte self-assigned this Jun 1, 2020
@agrare agrare force-pushed the allow_for_top_level_network_managers branch 2 times, most recently from 9bbfa4f to 2d8f6d1 Compare June 1, 2020 15:23
@agrare agrare requested a review from gtanzillo as a code owner June 1, 2020 15:23
@agrare agrare force-pushed the allow_for_top_level_network_managers branch from 2d8f6d1 to 4e4472e Compare June 1, 2020 16:27
@agrare agrare removed the unmergeable label Jun 1, 2020
@agrare
Copy link
Member Author

agrare commented Jun 1, 2020

@gtanzillo @lpichler question (re: #18504), the belongsto_descendant_class seems to only work on base-classes but not all NetworkManagers belong to a parent_manager.

Is there a way to get this find_object_by_special_class to respect sub-classes?

Extract the logic for a NetworkManager belonging to a CloudManager
parent out of the base NetworkManager class and move it to a mixin.
@agrare agrare force-pushed the allow_for_top_level_network_managers branch from 4e4472e to e01e164 Compare June 1, 2020 17:04
Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

Really like the factory changes.
Don't see where you are mixing this into NetworkManager (yes, WIP...)

These methods are white noise.
Wish we could delete them rather than hiding them behind a mixin.

virtual_total :total_vms_and_templates, :vms_and_templates

# Relationships delegated to parent manager
virtual_delegate :orchestration_stacks,
Copy link
Member

Choose a reason for hiding this comment

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

different PR:

would be nice if we used has_many :through instead.
Though, that will not work for virtual relationships on the manager model

@miq-bot
Copy link
Member

miq-bot commented Jun 2, 2020

Checked commits agrare/manageiq@e01e164~...832ac8a with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
4 files checked, 21 offenses detected

app/models/manageiq/providers/network_manager.rb

app/models/mixins/belongs_to_parent_manager_mixin.rb

@agrare
Copy link
Member Author

agrare commented Jun 2, 2020

Really like the factory changes.

Yeah I'm considering pulling just those out into its own PR since nuage and nsx-t having a parent manager isn't accurate even without this mixin

Don't see where you are mixing this into NetworkManager (yes, WIP...)

That's actually done in the provider PRs

@agrare agrare closed this Jun 24, 2020
@agrare agrare deleted the allow_for_top_level_network_managers branch June 24, 2020 16:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants