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.

Sign up for GitHub

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

Jump to bottom

Do not store openshift env in the controller, use lookup helper instead #1077

Merged
merged 2 commits into from
Apr 20, 2017
Merged

Do not store openshift env in the controller, use lookup helper instead #1077

merged 2 commits into from
Apr 20, 2017

Conversation

isimluk
Copy link
Member

@isimluk isimluk commented Apr 19, 2017 •
edited
Loading

@miq-bot miq-bot added the wip label Apr 19, 2017
kbrock
kbrock approved these changes Apr 19, 2017
View reviewed changes
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.

Reduced the number of queries and got rid of a controller @variable

LGTM

full disclosure: I did not run in the ui

app/helpers/report_helper/editor.rb
@@ -0,0 +1,12 @@
module ReportHelper::Editor
def cb_entities_by_provider_id(provider_id, entity_type)
provider = ManageIQ::Providers::ContainerManager.find(provider_id)
Copy link
Member

Choose a reason for hiding this comment

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

Did we just introduce an N+1 for looking up providers?

I guess if we were really concerned about this, we would use ContainerProject.where(:ems_id => provider_id)... - which is overkill at the moment.

The number of reduced queries for container_projects and container_images has been reduced, so I'm good with the change

Copy link
Member Author

Choose a reason for hiding this comment

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

I was aware, I decided it is good enough for the moment.

isimluk added 2 commits April 19, 2017 15:51
@isimluk
Do not store whole openshift env in the controller
4a65c34 
use lookup helper instead. Lookup only subset of the environment.

This should have been done in manageiq-ui-classic#532

Most of the time using session or instance variable is not encouraged.
@isimluk
Drop specs for the code I just removed.
ee2d8b3
@miq-bot
Copy link
Member

miq-bot commented Apr 19, 2017

Checked commits https://github.com/isimluk/manageiq-ui-classic/compare/4a65c3428dc82286958720e8d45bb81b707ec841~...ee2d8b3bd6c7a9c29ef832eb15a5d0356eace2d5 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
5 files checked, 0 offenses detected
Everything looks good. 🍪

@isimluk isimluk changed the title [WIP] Do not store openshift env in the controller, use lookup helper instead Do not store openshift env in the controller, use lookup helper instead Apr 19, 2017
@isimluk
Copy link
Member Author

isimluk commented Apr 19, 2017

@miq-bot remove_label wip
@miq-bot add_label reporting, chargeback, ui, bug, performance, scalability

@miq-bot
Copy link
Member

miq-bot commented Apr 19, 2017

@isimluk Cannot apply the following labels because they are not recognized: reporting, chargeback, ui, scalability

@martinpovolny martinpovolny merged commit 287dc37 into ManageIQ:master Apr 20, 2017
@martinpovolny martinpovolny added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 20, 2017
@martinpovolny martinpovolny self-assigned this Apr 20, 2017
@isimluk isimluk mentioned this pull request Apr 20, 2017
Merged
@isimluk isimluk deleted the rhbz#1442158 branch April 20, 2017 09:43
simaishi pushed a commit that referenced this pull request Apr 20, 2017
@martinpovolny @simaishi
Merge pull request #1077 from isimluk/rhbz#1442158
644a960 
Do not store openshift env in the controller, use lookup helper instead
(cherry picked from commit 287dc37)

https://bugzilla.redhat.com/show_bug.cgi?id=1444038
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit 644a960dd8288d4c408debdac275b9b742e918fc
Author: Martin Povolny <[email protected]>
Date:   Thu Apr 20 10:49:38 2017 +0200

    Merge pull request #1077 from isimluk/rhbz#1442158
    
    Do not store openshift env in the controller, use lookup helper instead
    (cherry picked from commit 287dc37c723d86d1e4fe6657a98e9dc29d3980d0)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1444038

This was referenced Apr 23, 2017
Closed
Closed
@simaishi
Copy link
Contributor

Euwe backport (to manageiq repo) details:

$ git log -1
commit 04883448a10fd06e89ff0b2440602178e878ae59
Author: Martin Povolny <[email protected]>
Date:   Thu Apr 20 10:49:38 2017 +0200

    Merge pull request #1077 from isimluk/rhbz#1442158
    
    Do not store openshift env in the controller, use lookup helper instead
    (cherry picked from commit 287dc37c723d86d1e4fe6657a98e9dc29d3980d0)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1444041

lpichler added a commit to lpichler/manageiq-ui-classic that referenced this pull request Jul 14, 2017
@lpichler
Remove includes as they are not needed after ManageIQ#1077
84274c5 
This is also fixing error about missing `ManageIQ::Providers::Openshift::ContainerManager::ContainerImage`
ManageIQ/manageiq-providers-openshift#23 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gtanzillo gtanzillo gtanzillo approved these changes

@kbrock kbrock kbrock approved these changes

Assignees

@martinpovolny martinpovolny

Labels
bug euwe/backported fine/backported overview/chargeback performance
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@isimluk @miq-bot @simaishi @kbrock @gtanzillo @martinpovolny @dclarizio