add binding to determine which method to use in relationship mixin #20274

d-m-u · 2020-06-12T13:27:50Z

we're trying to remove genealogy. since the relationship adding logic lives in the rel mixin, we need a way of differentiating that logic from actual ancestry logic so classes can use both.

based on the value of skip_relationships we need to either set an ancestry parent or a relationship parent

both parent= live on the ancestors that are proxies to each of the modules so you can call methods on any such ancestor with ruby magic wherein you can unbind a method and bind it to some other object.

@miq-bot assign @kbrock

kbrock

this looks great.

sorry, but I like the guard clause, it has less changes
yea, since the arguments are different for ancestry and this, we probably need to pass args
pass in ancestry parameters and not tge params that we require. I do wonder if we can drop the *_args parameters.
I'm a little worried about the differences in parent= - moving to update(:parent => ) or setting the parent when the vm is created may be our best bet
think batch refresh (openshift and at least 1 other) may feel this change

app/models/mixins/relationship_mixin.rb

lib/patches/ancestry_patch.rb

app/models/vm_or_template.rb

spec/models/mixins/relationship_mixin_spec.rb

app/models/mixins/relationship_mixin.rb

in context of ManageIQ/manageiq-schema#492

d-m-u · 2020-06-14T03:28:42Z

@kbrock you were right about the args getting stomped, it's somewhere around https://github.com/matthewrudy/memoist/blob/v0.15.0/lib/memoist.rb#L213 which is the reason why i have that gook

kbrock

thanks for these changes - this looks great.

Anything outstanding or can we un-WIP it?

kbrock · 2020-10-03T02:29:17Z

app/models/mixins/relationship_mixin.rb

 parent.with_relationship_type(relationship_type) { parent.add_child(self) }
 end

 def add_children(*child_objs)
+ return child_objs.each { |child| child.with_relationship_type(relationship_type) { child.update!(:parent => self) } } if use_ancestry?
+


well regardless, either the child will have the wrong parent or the parent will have the wrong children. This is the nature of active record in general. I'm ok with this line.

app/models/mixins/relationship_mixin.rb

kbrock · 2020-10-03T02:39:44Z

app/models/mixins/relationship_mixin.rb

 ancestor_ids.include?([obj.class.base_class.name, obj.id])
 end

 def is_ancestor_of?(obj)
+ return obj.with_relationship_type(relationship_type) { obj.descendant_of?(self) } if use_ancestry?


looking up descendant ids is a database call but looking up ancestors is not.

so calling is_descendant_of requires fewer queries.

app/models/mixins/relationship_mixin.rb

d-m-u · 2020-10-24T03:09:26Z

marking as work in progress because i'm concerned about the behavior of the class variables:

2.5.5 :001 > Dialog.hidden_attribute_names
 => []
2.5.5 :002 > Vm.hidden_attribute_names
 => []
2.5.5 :003 > ExtManagementSystem.hidden_attribute_names
 => ["aggregate_memory"]
2.5.5 :004 > Vm.hidden_attribute_names
 => ["aggregate_memory"]
2.5.5 :005 > Dialog.hidden_attribute_names
 => ["aggregate_memory"]

see #20664 (comment) for the details

d-m-u · 2020-11-01T01:11:13Z

un-work in progressing because the cattr_accessor concern was resolved, see https://github.com/ManageIQ/manageiq/pull/20664/files#r513793189, thanks Jason

d-m-u · 2020-11-01T01:38:22Z

@kbrock could I please get one more review here? 🤞

kbrock

I would like to move ahead with this one. we just branched off K and something risky like this needs to be done at the beginning of a release cycle - if we are lucky, then we will be able to remove the need for this by changing all the classes

app/models/mixins/relationship_mixin.rb

kbrock · 2020-11-02T01:49:39Z

app/models/mixins/relationship_mixin.rb

@@ -26,6 +27,7 @@ module RelationshipMixin
 extend Memoist

 cattr_accessor :default_relationship_type
+ class_attribute :skip_relationships, :default => []


this looks great - although, not sure if we want this to carry across all the classes. Will we tell all classes to skip ancestry that are in the chain? If yes, then this is great.

based on the value of default_relationship_type we need to either set an ancestry parent or a relationship parent as vms use both both parent= live on the ancestors that are proxies to each of the modules so you can call methods on any such ancestor with some black magic wherein you can unbind a method and bind it to some other object

miq-bot · 2020-11-02T02:10:24Z

Checked commit d-m-u@6fefedb with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
2 files checked, 36 offenses detected

spec/models/mixins/relationship_mixin_spec.rb

❗ - Line 230, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 234, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 238, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 242, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 246, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 250, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 254, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 258, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 262, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 266, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 270, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 274, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 278, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 282, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 286, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 290, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 294, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 298, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 302, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 306, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 310, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 314, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 318, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 322, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 326, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 330, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 334, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 338, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 342, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 343, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 348, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 350, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 355, Col 75 - Layout/SpaceAroundOperators - Surrounding space missing for operator =.
❗ - Line 355, Col 76 - Style/RedundantParentheses - Don't use parentheses around a variable.
❗ - Line 355, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
❗ - Line 357, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.

d-m-u requested review from agrare, Fryguy and kbrock as code owners June 12, 2020 13:27

miq-bot added the wip label Jun 12, 2020

d-m-u force-pushed the use_rel_type_to_differentiate_between_module_methods_in_mixin branch from 20d1e2e to d3298cd Compare June 12, 2020 13:37

miq-bot assigned kbrock Jun 12, 2020

d-m-u force-pushed the use_rel_type_to_differentiate_between_module_methods_in_mixin branch from d3298cd to 0df8369 Compare June 12, 2020 17:36

d-m-u requested a review from gtanzillo as a code owner June 12, 2020 17:36

d-m-u force-pushed the use_rel_type_to_differentiate_between_module_methods_in_mixin branch 6 times, most recently from 3744bce to ccc2cb4 Compare June 13, 2020 05:55

kbrock reviewed Jun 13, 2020

View reviewed changes

d-m-u force-pushed the use_rel_type_to_differentiate_between_module_methods_in_mixin branch from ccc2cb4 to 53f689a Compare June 13, 2020 18:04

d-m-u closed this Jun 13, 2020

d-m-u reopened this Jun 13, 2020

d-m-u force-pushed the use_rel_type_to_differentiate_between_module_methods_in_mixin branch 3 times, most recently from 596af70 to 55e779a Compare June 13, 2020 23:14

d-m-u added a commit to d-m-u/manageiq-cross_repo-tests that referenced this pull request Jun 14, 2020

https://github.com/ManageIQ/manageiq/pull/20274

5203eb8

in context of ManageIQ/manageiq-schema#492

d-m-u mentioned this pull request Jun 14, 2020

test vms with ancestry ManageIQ/manageiq-cross_repo-tests#141

Closed

d-m-u added a commit to d-m-u/manageiq-cross_repo-tests that referenced this pull request Jun 14, 2020

https://github.com/ManageIQ/manageiq/pull/20274

4c6c236

in context of ManageIQ/manageiq-schema#492

d-m-u mentioned this pull request Jun 14, 2020

test openshift in context of vm ancestry prs in core & schema ManageIQ/manageiq-cross_repo-tests#142

Closed

d-m-u force-pushed the use_rel_type_to_differentiate_between_module_methods_in_mixin branch from 55e779a to 2f6afdb Compare June 14, 2020 03:05

d-m-u force-pushed the use_rel_type_to_differentiate_between_module_methods_in_mixin branch 3 times, most recently from c55a649 to d5daeea Compare June 14, 2020 17:11

d-m-u mentioned this pull request Oct 2, 2020

add ancestry case to the genealogy parent setter #20642

Merged

kbrock approved these changes Oct 3, 2020

View reviewed changes

d-m-u changed the title ~~[wip] add binding to determine which method to use in relationship mixin~~ add binding to determine which method to use in relationship mixin Oct 3, 2020

miq-bot removed the wip label Oct 3, 2020

kbrock requested a review from Fryguy October 4, 2020 02:00

chessbyte added the refactoring label Oct 6, 2020

d-m-u changed the title ~~add binding to determine which method to use in relationship mixin~~ [WIP] add binding to determine which method to use in relationship mixin Oct 24, 2020

miq-bot added the wip label Oct 24, 2020

kbrock self-requested a review October 26, 2020 01:59

d-m-u force-pushed the use_rel_type_to_differentiate_between_module_methods_in_mixin branch from 2be70a8 to de934a3 Compare November 1, 2020 01:10

d-m-u changed the title ~~[WIP] add binding to determine which method to use in relationship mixin~~ add binding to determine which method to use in relationship mixin Nov 1, 2020

miq-bot removed the wip label Nov 1, 2020

d-m-u force-pushed the use_rel_type_to_differentiate_between_module_methods_in_mixin branch from de934a3 to d56ac01 Compare November 1, 2020 01:38

d-m-u force-pushed the use_rel_type_to_differentiate_between_module_methods_in_mixin branch from d56ac01 to d92e48c Compare November 1, 2020 01:56

kbrock reviewed Nov 2, 2020

View reviewed changes

d-m-u force-pushed the use_rel_type_to_differentiate_between_module_methods_in_mixin branch from d92e48c to 6fefedb Compare November 2, 2020 02:07

kbrock merged commit 9cd54e7 into ManageIQ:master Nov 5, 2020

d-m-u deleted the use_rel_type_to_differentiate_between_module_methods_in_mixin branch November 5, 2020 13:31

This was referenced Jan 28, 2021

use ancestry in replace_children #20990

Merged

save after setting parent on vm update ManageIQ/manageiq-api#994

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add binding to determine which method to use in relationship mixin #20274

add binding to determine which method to use in relationship mixin #20274

d-m-u commented Jun 12, 2020 •

edited

Loading

kbrock left a comment •

edited

Loading

d-m-u commented Jun 14, 2020 •

edited

Loading

kbrock left a comment

kbrock Oct 3, 2020

kbrock Oct 3, 2020

d-m-u commented Oct 24, 2020 •

edited

Loading

d-m-u commented Nov 1, 2020

d-m-u commented Nov 1, 2020

kbrock left a comment

kbrock Nov 2, 2020

miq-bot commented Nov 2, 2020

add binding to determine which method to use in relationship mixin #20274

add binding to determine which method to use in relationship mixin #20274

Conversation

d-m-u commented Jun 12, 2020 • edited Loading

kbrock left a comment • edited Loading

Choose a reason for hiding this comment

d-m-u commented Jun 14, 2020 • edited Loading

kbrock left a comment

Choose a reason for hiding this comment

kbrock Oct 3, 2020

Choose a reason for hiding this comment

kbrock Oct 3, 2020

Choose a reason for hiding this comment

d-m-u commented Oct 24, 2020 • edited Loading

d-m-u commented Nov 1, 2020

d-m-u commented Nov 1, 2020

kbrock left a comment

Choose a reason for hiding this comment

kbrock Nov 2, 2020

Choose a reason for hiding this comment

miq-bot commented Nov 2, 2020

d-m-u commented Jun 12, 2020 •

edited

Loading

kbrock left a comment •

edited

Loading

d-m-u commented Jun 14, 2020 •

edited

Loading

d-m-u commented Oct 24, 2020 •

edited

Loading