manageiq icon indicating copy to clipboard operation
manageiq copied to clipboard

[WIP] Don't `belongs_to :parent_manager` in base manager classes

Open agrare opened this issue 3 years ago • 6 comments

The base NetworkManager / StorageManager class shouldn't belong to a parent_manager as this makes it significantly more difficult to have standalone managers of those types.

agrare avatar May 05 '21 18:05 agrare

Is it possible to remove the delegation to parent managers in general? Whether they are virtual delegates, regular delegates, or belongs_to, they all make things so inefficient and convoluted.

kbrock avatar May 11 '21 03:05 kbrock

Checked commit https://github.com/agrare/manageiq/commit/1164861c94d665d15cbba56df5bd98bc4aaa80f2 with ruby 2.6.3, rubocop 1.13.0, haml-lint 0.35.0, and yamllint 5 files checked, 19 offenses detected

app/models/mixins/belongs_to_parent_manager_mixin.rb

  • [ ] :exclamation: - Line 5, Col 5 - Rails/InverseOf - Specify an :inverse_of option.

app/models/mixins/child_network_manager_mixin.rb

  • [ ] :exclamation: - Line 10, Col 5 - Rails/InverseOf - Specify an :inverse_of option.
  • [ ] :exclamation: - Line 11, Col 5 - Rails/InverseOf - Specify an :inverse_of option.
  • [ ] :exclamation: - Line 12, Col 5 - Rails/InverseOf - Specify an :inverse_of option.
  • [ ] :exclamation: - Line 13, Col 5 - Rails/InverseOf - Specify an :inverse_of option.
  • [ ] :exclamation: - Line 14, Col 5 - Rails/InverseOf - Specify an :inverse_of option.
  • [ ] :exclamation: - Line 15, Col 5 - Rails/InverseOf - Specify an :inverse_of option.
  • [ ] :exclamation: - Line 16, Col 5 - Rails/InverseOf - Specify an :inverse_of option.
  • [ ] :exclamation: - Line 17, Col 5 - Rails/InverseOf - Specify an :inverse_of option.
  • [ ] :exclamation: - Line 18, Col 5 - Rails/InverseOf - Specify an :inverse_of option.
  • [ ] :exclamation: - Line 19, Col 5 - Rails/InverseOf - Specify an :inverse_of option.
  • [ ] :exclamation: - Line 20, Col 5 - Rails/InverseOf - Specify an :inverse_of option.
  • [ ] :exclamation: - Line 21, Col 5 - Rails/InverseOf - Specify an :inverse_of option.
  • [ ] :exclamation: - Line 22, Col 5 - Rails/InverseOf - Specify an :inverse_of option.
  • [ ] :exclamation: - Line 23, Col 5 - Rails/InverseOf - Specify an :inverse_of option.
  • [ ] :exclamation: - Line 24, Col 5 - Rails/InverseOf - Specify an :inverse_of option.
  • [ ] :exclamation: - Line 25, Col 5 - Rails/InverseOf - Specify an :inverse_of option.
  • [ ] :exclamation: - Line 26, Col 5 - Rails/InverseOf - Specify an :inverse_of option.
  • [ ] :exclamation: - Line 9, Col 5 - Rails/InverseOf - Specify an :inverse_of option.

miq-bot avatar May 12 '21 16:05 miq-bot

Is it possible to remove the delegation to parent managers in general?

As long as we have a "parent" cloud_manager which holds the credentials and one-to-many "child" managers we'll at least need to delegate all of the connection methods to the parent manager (or duplicate the creds across the child managers and keep in sync which seems worse).

agrare avatar May 12 '21 17:05 agrare

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

miq-bot avatar May 26 '21 21:05 miq-bot

When dealing with screens, it is tricky to have a relationship that is defined in a subclass.

kbrock avatar Jun 04 '21 17:06 kbrock

I misspoke. Delegating connection and keeping credentials works great. It is the delegations that are used to search, order, or display something on the screen that would be nice to avoid.

best scenario is for all classes to act the same for all parents and children. E.g.: all of them have zone_id, or the same relationship defined.

If we have screens that only target infra, then putting infra only associations there is good too. But we run into problems when we search on the parent providers and those overwritten attributes/relationships are used.

These one off managers that have a special way to determine information that is a different way from others will cause the screens to behave in peculiar ways. And happen to not perform as well.

thanks for all the help getting rid of these delegates and duplicating values from parent managers to child managers.

kbrock avatar Jul 24 '21 14:07 kbrock

This pull request has been automatically closed because it has not been updated for at least 3 months.

Feel free to reopen this pull request if these changes are still valid.

Thank you for all your contributions! More information about the ManageIQ triage process can be found in the triage process documentation.

miq-bot avatar Feb 27 '23 00:02 miq-bot

@agrare I think merged the PRs that made sure the zones/ems and others are propagated from parent to child managers.

So have we already solved this PR? Or have we done all the work and just need to merge this? Or is there probably stuff outstanding and we have higher priorities?

kbrock avatar Mar 15 '23 00:03 kbrock