nix the string interpolation on the hardware query in the aggregation mixin #20146

d-m-u · 2020-05-08T01:43:49Z

It's just a minor cleanup from #20132 cause I was silly and didn't catch it the first time.

Or rather it was. I'm stealing a lot of this now from master...kbrock:virtual_aggregates_ems

thanks Keenan

app/models/mixins/aggregation_mixin/methods.rb

Fryguy · 2020-05-08T01:56:47Z

LGTM... super minor, but since there's no changes in the value of from from when it's declared, it might be cleaner to move the .to_sym earlier to the declaration....

from      = from.to_s.singularize.to_sym
...
hdws      = Hardware.where(from => targets).select(select)

d-m-u · 2020-05-08T02:04:09Z

@miq-bot add_label jansa/yes?
it should be, i would hope, since #20132 was

kbrock

Thanks for putting this together.

You may want to check out master...kbrock:virtual_aggregates_ems

ugh. that was 7 months ago... (too many ideas :( )

leave the inject in there for now, but maybe the other optimizations may treat you well.

Also, could you just do a quick check that this method is even called from tests.
Maybe put in a raise statement or something?

thnx

app/models/mixins/aggregation_mixin/methods.rb

kbrock · 2020-05-08T06:43:52Z

aside: that branch I pointed to does all metrics in 1 query bringing back only 1 number.
the reason I haven't pushed harder is I'm concerned about number overflows - especially with fields that return memory/disk space in bytes. (can't remember exactly which one was a number but it went over, thanks to NickL on finding that one.)

per ManageIQ#20146 (comment) I stole most of this from ManageIQ/manageiq@master...kbrock:virtual_aggregates_ems thanks Keenan and Jason :)

Things changed

d-m-u · 2020-05-08T13:15:36Z

haha, Things changed
... and probably not for the better

d-m-u · 2020-05-09T03:53:41Z

~~closing in favor of #12427~~

kbrock · 2020-05-11T15:55:29Z

wait - no. Your pr doesn't go the whole way so i want to merge this instead of that. We still have an overflow issue with 12427 and we won't fix that any time soon

d-m-u · 2020-05-11T15:56:53Z

phoooey, it was a good try though

kbrock

nice. (but I helped a little here so it would be a problem if I though it was junk)

app/models/mixins/aggregation_mixin/methods.rb

d-m-u · 2020-05-12T16:40:55Z

hey @Fryguy sorry but any chance I could get you to re-review this, please?

thanks Keenan and Jason :)

miq-bot · 2020-05-12T18:30:26Z

Checked commit d-m-u@e342471 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
3 files checked, 0 offenses detected
Everything looks fine. 🍪

d-m-u · 2020-05-13T15:57:51Z

please?

nix the string interpolation on the hardware query in the aggregation mixin (cherry picked from commit 9be2b9b)

simaishi · 2020-05-18T18:55:22Z

Jansa backport details:

$ git log -1
commit b7a41372d8ff06296799301a247e9ea8f9926ced
Author: Keenan Brock <keenan@thebrocks.net>
Date:   Wed May 13 22:04:39 2020 -0400

    Merge pull request #20146 from d-m-u/cleanup_the_hardware_query

    nix the string interpolation on the hardware query in the aggregation mixin

    (cherry picked from commit 9be2b9bf65589d307d1ee246199ff488fedf8844)

d-m-u requested review from agrare, Fryguy and kbrock as code owners May 8, 2020 01:43

d-m-u changed the title ~~nix the string interpolation~~ nix the string interpolation on the hardware query in the aggregation mixin May 8, 2020

Fryguy reviewed May 8, 2020

View reviewed changes

app/models/mixins/aggregation_mixin/methods.rb Outdated Show resolved Hide resolved

Fryguy approved these changes May 8, 2020

View reviewed changes

Fryguy added the cleanup label May 8, 2020

Fryguy self-assigned this May 8, 2020

d-m-u force-pushed the cleanup_the_hardware_query branch from 119ee3e to ba56cfa Compare May 8, 2020 01:59

Fryguy previously approved these changes May 8, 2020

View reviewed changes

miq-bot added the jansa/yes? label May 8, 2020

kbrock reviewed May 8, 2020

View reviewed changes

app/models/mixins/aggregation_mixin/methods.rb Outdated Show resolved Hide resolved

app/models/mixins/aggregation_mixin/methods.rb Outdated Show resolved Hide resolved

d-m-u changed the title ~~nix the string interpolation on the hardware query in the aggregation mixin~~ [wip] nix the string interpolation on the hardware query in the aggregation mixin May 8, 2020

miq-bot added the wip label May 8, 2020

d-m-u force-pushed the cleanup_the_hardware_query branch from ba56cfa to 727f637 Compare May 8, 2020 11:24

d-m-u requested a review from gtanzillo as a code owner May 8, 2020 11:24

d-m-u added a commit to d-m-u/manageiq that referenced this pull request May 8, 2020

nix the string interpolation

d7e0121

per ManageIQ#20146 (comment) I stole most of this from ManageIQ/manageiq@master...kbrock:virtual_aggregates_ems thanks Keenan and Jason :)

d-m-u force-pushed the cleanup_the_hardware_query branch from 727f637 to d7e0121 Compare May 8, 2020 11:33

d-m-u added a commit to d-m-u/manageiq that referenced this pull request May 8, 2020

nix the string interpolation

7d51f60

per ManageIQ#20146 (comment) I stole most of this from ManageIQ/manageiq@master...kbrock:virtual_aggregates_ems thanks Keenan and Jason :)

d-m-u force-pushed the cleanup_the_hardware_query branch from d7e0121 to 7d51f60 Compare May 8, 2020 12:01

d-m-u added a commit to d-m-u/manageiq that referenced this pull request May 8, 2020

nix the string interpolation

1864591

per ManageIQ#20146 (comment) I stole most of this from ManageIQ/manageiq@master...kbrock:virtual_aggregates_ems thanks Keenan and Jason :)

d-m-u force-pushed the cleanup_the_hardware_query branch from 7d51f60 to 1864591 Compare May 8, 2020 12:27

Fryguy assigned kbrock and unassigned Fryguy May 8, 2020

d-m-u force-pushed the cleanup_the_hardware_query branch 2 times, most recently from 3658d3e to 402c497 Compare May 8, 2020 12:51

d-m-u changed the title ~~[wip] nix the string interpolation on the hardware query in the aggregation mixin~~ nix the string interpolation on the hardware query in the aggregation mixin May 8, 2020

miq-bot removed the wip label May 8, 2020

d-m-u closed this May 9, 2020

d-m-u reopened this May 11, 2020

kbrock approved these changes May 11, 2020

View reviewed changes

kbrock reviewed May 11, 2020

View reviewed changes

app/models/mixins/aggregation_mixin/methods.rb Show resolved Hide resolved

d-m-u force-pushed the cleanup_the_hardware_query branch from 402c497 to f7b20d5 Compare May 11, 2020 18:05

d-m-u force-pushed the cleanup_the_hardware_query branch from f7b20d5 to ab7cedd Compare May 12, 2020 18:23

nix the string interpolation

e342471

thanks Keenan and Jason :)

d-m-u force-pushed the cleanup_the_hardware_query branch from ab7cedd to e342471 Compare May 12, 2020 18:26

kbrock added the core label May 14, 2020

kbrock merged commit 9be2b9b into ManageIQ:master May 14, 2020

d-m-u deleted the cleanup_the_hardware_query branch May 14, 2020 02:06

kbrock mentioned this pull request May 14, 2020

[WIP] Use aggregates for aggregate_mixin #12427

Closed

4 tasks

gtanzillo added jansa/yes and removed jansa/yes? labels May 14, 2020

d-m-u mentioned this pull request May 15, 2020

change aggregation mixin methods into virtual attributes #20149

Merged

simaishi pushed a commit that referenced this pull request May 18, 2020

Merge pull request #20146 from d-m-u/cleanup_the_hardware_query

b7a4137

nix the string interpolation on the hardware query in the aggregation mixin (cherry picked from commit 9be2b9b)

simaishi added jansa/backported and removed jansa/yes labels May 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nix the string interpolation on the hardware query in the aggregation mixin #20146

nix the string interpolation on the hardware query in the aggregation mixin #20146

d-m-u commented May 8, 2020 •

edited

Loading

Fryguy commented May 8, 2020

d-m-u commented May 8, 2020 •

edited

Loading

kbrock left a comment •

edited

Loading

kbrock commented May 8, 2020

d-m-u commented May 8, 2020

d-m-u commented May 9, 2020 •

edited

Loading

kbrock commented May 11, 2020

d-m-u commented May 11, 2020 •

edited

Loading

kbrock left a comment

d-m-u commented May 12, 2020

miq-bot commented May 12, 2020

d-m-u commented May 13, 2020

simaishi commented May 18, 2020

nix the string interpolation on the hardware query in the aggregation mixin #20146

nix the string interpolation on the hardware query in the aggregation mixin #20146

Conversation

d-m-u commented May 8, 2020 • edited Loading

Fryguy commented May 8, 2020

d-m-u commented May 8, 2020 • edited Loading

kbrock left a comment • edited Loading

Choose a reason for hiding this comment

kbrock commented May 8, 2020

d-m-u commented May 8, 2020

d-m-u commented May 9, 2020 • edited Loading

kbrock commented May 11, 2020

d-m-u commented May 11, 2020 • edited Loading

kbrock left a comment

Choose a reason for hiding this comment

d-m-u commented May 12, 2020

miq-bot commented May 12, 2020

d-m-u commented May 13, 2020

simaishi commented May 18, 2020

d-m-u commented May 8, 2020 •

edited

Loading

d-m-u commented May 8, 2020 •

edited

Loading

kbrock left a comment •

edited

Loading

d-m-u commented May 9, 2020 •

edited

Loading

d-m-u commented May 11, 2020 •

edited

Loading