These are chat archives for ManageIQ/manageiq/performance

22nd
Mar 2018
Keenan Brock
@kbrock
Mar 22 2018 12:22
@cben putting together specs for current behavior today. Want to see what I can tear out
But being in so many places, it is a little intimidating
Nick LaMuro
@NickLaMuro
Mar 22 2018 15:25

Okay... can someone tell me if I am missing something here or this is wrong:

https://github.com/ManageIQ/manageiq/blob/d885f8c6d/app/models/miq_report/generator.rb#L228

In the case that code path is hit, does that mean that #build_table is called twice then (redundantly)??

Keenan Brock
@kbrock
Mar 22 2018 15:28
ha - that looks like a bug
some of the reports do tons of logic internally. and some reports are 2 pass
Joe Rafaniello
@jrafanie
Mar 22 2018 15:29
WAT, yeah, that looks wrong
Keenan Brock
@kbrock
Mar 22 2018 15:29
if it were a 2 pass, I would have expected it to manipulate the result value
@NickLaMuro I'm scared to do a git blame of that stuff
Nick LaMuro
@NickLaMuro
Mar 22 2018 15:30
it's Matthew, so you are good ;)
Keenan Brock
@kbrock
Mar 22 2018 15:30
lol. well, his is a fix all rubocops
that doesn't put me in the clear
Nick LaMuro
@NickLaMuro
Mar 22 2018 15:30
yeah... let me double check
I was just about to say that too :smile:
Keenan Brock
@kbrock
Mar 22 2018 15:31
when I was trying to get scopes in there, 3? years ago, I moved some stuff around
by "some" I mean a non trivial amount that seemed out of control
Nick LaMuro
@NickLaMuro
Mar 22 2018 15:32
Oleg, it seems
Keenan Brock
@kbrock
Mar 22 2018 15:32
guess we'll keep him anyway
Nick LaMuro
@NickLaMuro
Mar 22 2018 15:32
though, that was just a move... :mag:
Keenan Brock
@kbrock
Mar 22 2018 15:33
no
if it was oleg, then I'm in the clear
well, maybe I was suposed to delete that when refactoring, but that seems due diligence by now
@NickLaMuro are these the type of specs you were thinking? ManageIQ/manageiq#17186
in regards to specs for the cli tools
look ma. no puts
Nick LaMuro
@NickLaMuro
Mar 22 2018 15:38
:+1:
Nick LaMuro
@NickLaMuro
Mar 22 2018 15:44
I am surprised you left my tests in there to be fair
Keenan Brock
@kbrock
Mar 22 2018 15:46
kinda an integration test
Nick LaMuro
@NickLaMuro
Mar 22 2018 20:40

So... I guess this is the "Shi... stuff Nick finds in MiqReport" day, but is this line not needed any more?

https://github.com/ManageIQ/manageiq/blob/master/app/models/miq_report/generator.rb#L407

Basically, looks like when it was useful was when prior to commit e8dcb780ecbba2d2e9ace20fe39936c5648d38ff (sorry non-cfme devs, you can't see this), before the pivot stuff was being introduced. But now, because of how Ruport's code works, basically we are just cloning the Ruport::Data::Table object with the exact same columns we defined it with in the line above...

Nick LaMuro
@NickLaMuro
Mar 22 2018 23:06

Somewhat in response the previous conversation from above, I spent some time today refactoring the MiqReport::Generator#_generate_table method here:

ManageIQ/manageiq#17189

Part of what is included is removing the extraneous build_table call