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

Proper parent initialization #14209

Merged
merged 2 commits into from
Mar 7, 2017

Conversation

Ladas
Copy link
Contributor

@Ladas Ladas commented Mar 7, 2017

Set :parent to manager only if not sent in options and explicitly pass :parent => nil, because it has a priority over :arel in the DB strategies.

So e.g. for AWS, when we use a DB strategy, it's enough to use parent and association, since the related Vm can only be in the scope of manager.vms. But for Ansible, the crosslink can be to any VM, therefore we use a generic arel to get to the right Vm.

The priority for DB strategy starting with higher priority is: :custom_db_finder, :parent & :association, :arel. So the first defined is used for fetching data from the DB.

Ladas added 2 commits March 7, 2017 12:01
Set manager only if not sent in options
Explicitly pass :parent => nil, because it has a priority over
:arel in the DB strategies.
@Ladas
Copy link
Contributor Author

Ladas commented Mar 7, 2017

@miq-bot assign @agrare

@miq-bot
Copy link
Member

miq-bot commented Mar 7, 2017

Checked commits Ladas/manageiq@ff28bce~...9265a8b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 🍰

@durandom
Copy link
Member

durandom commented Mar 7, 2017

could you try to update the description to be more explanatory?
I guess I need this when it comes to documentation
e.g. is there a case where you want to pass a parent although strategy is :db ?
and what do you mean by

has a priority over :arel in the DB strategies.

@durandom
Copy link
Member

durandom commented Mar 7, 2017

The priorities are here

def db_relation(selection = nil, projection = nil)
relation = if !custom_db_finder.blank?
custom_db_finder.call(self, selection, projection)
else
rel = if !parent.nil? && !association.nil?
parent.send(association)
elsif !arel.nil?
arel
end
rel = rel.where(selection) if rel && selection
rel = rel.select(projection) if rel && projection
rel
end
relation || []
end

Copy link
Member

@durandom durandom left a comment

Choose a reason for hiding this comment

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

ok. I think what confuses / annoys me, is that we have to pass something as nil
Maybe defaulting the :parent to manager is not so good...

👍 until we see a lot of :parent => nil

@agrare
Copy link
Member

agrare commented Mar 7, 2017

Are there any specs around cross-linking? Seems this should have caused a test failure with good test coverage

@durandom
Copy link
Member

durandom commented Mar 7, 2017

@agrare this is just the DSL and only ansible tower is using it. And the DSL is only syntactic sugar.

Though I agree we should move the spec to use the DSL soon.

re-assigning to @kbrock as he merged the original PR

@durandom
Copy link
Member

durandom commented Mar 7, 2017

@miq-bot assign @kbrock

@miq-bot miq-bot assigned kbrock and unassigned agrare Mar 7, 2017
@kbrock kbrock added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 7, 2017
@kbrock
Copy link
Member

kbrock commented Mar 7, 2017

looks to handle the use case well. thanks ladas

@kbrock kbrock merged commit 0b5e558 into ManageIQ:master Mar 7, 2017
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.

6 participants