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

Vim performance scope #7260

Merged
merged 10 commits into from
Mar 18, 2016
Merged

Vim performance scope #7260

merged 10 commits into from
Mar 18, 2016

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Mar 14, 2016

This is focused on VimPerformanceDaily.find_entries

The goal is to remove allow scopes to be used instead. Rbac will be able to convert VimPerformanceDaily into a scope and much of the specialized logic around scope and klass can be removed.

After this, think we can pull klass lookup out of find_entries
Let me know the best way to code the OR if you think I should convert that too.

/cc @matthewd This is a follow up to our conversation.

@chessbyte
Copy link
Member

@gtanzillo please review


if obj.kind_of?(MiqEnterprise) || obj.kind_of?(MiqRegion)
cond1 = klass.send(:sanitize_sql_for_conditions, :resource_type => "Storage", :resource_id => obj.storage_ids)
cond2 = klass.send(:sanitize_sql_for_conditions, :resource_type => "ExtManagementSystem", :resource_id => obj.ext_management_system_ids)
cond += " AND ((#{cond1}) OR (#{cond2}))"
rel = rel.where("((#{cond1}) OR (#{cond2}))")
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're here, you can or this, and thus kill the sanitize calls.

Copy link
Member Author

Choose a reason for hiding this comment

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

@matthewd that was my question - what does this OR code look like?

Copy link
Contributor

Choose a reason for hiding this comment

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

rel = rel.where(resource: obj.storages).or(rel.where(resource: obj.ext_management_systems))

@kbrock kbrock mentioned this pull request Mar 14, 2016
@miq-bot
Copy link
Member

miq-bot commented Mar 15, 2016

<pr_mergeability_checker />This pull request is not mergeable. Please rebase and repush.

@kbrock kbrock changed the title Vim performance scope [WIP]Vim performance scope Mar 15, 2016
@kbrock
Copy link
Member Author

kbrock commented Mar 15, 2016

WIPping until I get a test into here

@kbrock kbrock added the wip label Mar 15, 2016
@chessbyte chessbyte changed the title [WIP]Vim performance scope [WIP] Vim performance scope Mar 15, 2016
@kbrock kbrock force-pushed the vim_performance_scope branch 2 times, most recently from 19af989 to 81fd207 Compare March 17, 2016 18:23
@kbrock
Copy link
Member Author

kbrock commented Mar 17, 2016

  • now using AR OR
  • changed :resource to leverage rails.
  • added basic spec for vim_performance_analysis

@kbrock kbrock removed the wip label Mar 17, 2016
@kbrock kbrock changed the title [WIP] Vim performance scope Vim performance scope Mar 17, 2016
@kbrock
Copy link
Member Author

kbrock commented Mar 17, 2016

fixed rubocops.

@matthewd Think I addressed all your concerns/suggestions.

Followup PR extracts the scopes and sunsets find_entries

@miq-bot
Copy link
Member

miq-bot commented Mar 17, 2016

Checked commits kbrock/manageiq@7aa7690~...ef808e3 with ruby 2.2.3, rubocop 0.37.2, and haml-lint 0.16.1
7 files checked, 3 offenses detected

app/models/miq_report/generator/utilization.rb

spec/models/vim_performance_analysis_spec.rb

matthewd added a commit that referenced this pull request Mar 18, 2016
@matthewd matthewd merged commit 73abbc6 into ManageIQ:master Mar 18, 2016
@matthewd matthewd added this to the Sprint 38 Ending Mar 28, 2016 milestone Mar 18, 2016
@kbrock kbrock deleted the vim_performance_scope branch March 18, 2016 21:14
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