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

Check all costs fields for relevancy for the report #17387

Merged
merged 1 commit into from
May 8, 2018
Merged

Check all costs fields for relevancy for the report #17387

merged 1 commit into from
May 8, 2018

Conversation

jrafanie
Copy link
Member

@jrafanie jrafanie commented May 4, 2018 •
edited
Loading

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1567254

If a user asks only for some of the fields (memory cost) in a chargeback report
and they didn't request specific ones (memory allocated cost), we would skip
calculating the memory cost because we thought it wasn't relevant. We need to
check all cost fields to see if they're relevant for the requested fields in the
report.

@jrafanie jrafanie requested a review from lpichler May 4, 2018 21:39
@jrafanie
Check all costs fields for relevancy for the report
4b15db4 
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1567254

If a user asks only for some of the fields (memory cost) in a chargeback report
and they didn't request specific ones (memory allocated cost), we would skip
calculating the memory cost because we thought it wasn't relevant.  We need to
check all cost fields to see if they're relevant for the requested fields in the
report.
@jrafanie jrafanie force-pushed the bring_in_relevant_fields_needed_for_charge_calculation branch from 262221e to 4b15db4 Compare May 4, 2018 21:55
@jrafanie jrafanie changed the title Bring in relevant fields needed for charge Check all costs fields for relevancy for the report May 4, 2018
@jrafanie
Copy link
Member Author

jrafanie commented May 4, 2018

I believe this will require a manual backport to the different branches. I'll check that on Monday.

@miq-bot
Copy link
Member

miq-bot commented May 4, 2018

Checked commit jrafanie@4b15db4 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

lpichler
lpichler approved these changes May 7, 2018
View reviewed changes
Copy link
Contributor

@lpichler lpichler left a comment

Choose a reason for hiding this comment

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

very good thanks! LGTM 👍

Note: this change is also fixing general total_cost column, so maybe it would be beneficial to add other it case when only column
subject.total_cost is requested.

expect(subject.total_cost).to eq(_)..

thanks!

@lpichler
Copy link
Contributor

lpichler commented May 7, 2018

I believe this will require a manual backport to the different branches. I'll check that on Monday.

most likely caused by this:
#12885

@jrafanie
Copy link
Member Author

jrafanie commented May 7, 2018

Note: this change is also fixing general total_cost column, so maybe it would be beneficial to add other it case when only column
subject.total_cost is requested.

expect(subject.total_cost).to eq(_)..

@lpichler I need to test various combinations:

  1. memory cost and total cost in a report (total cost should equal memory cost)
  2. memory cost, cpu cost and total cost in a report(total cost should equal memory cost + cpu cost)
  3. total cost by itself (should be blank)

gtanzillo
gtanzillo approved these changes May 8, 2018
View reviewed changes
Copy link
Member

@gtanzillo gtanzillo left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@gtanzillo gtanzillo added this to the Sprint 85 Ending May 7, 2018 milestone May 8, 2018
@gtanzillo gtanzillo merged commit 1588636 into ManageIQ:master May 8, 2018
@jrafanie jrafanie deleted the bring_in_relevant_fields_needed_for_charge_calculation branch May 8, 2018 20:26
@jrafanie
Copy link
Member Author

jrafanie commented May 8, 2018

@miq-bot add_label gaprindashvili/yes

@jrafanie
Copy link
Member Author

jrafanie commented May 8, 2018

It isn't a clean cherry-pick to Fine, in fact some might even say it's not Fine. I'll open a PR for Fine.

jrafanie added a commit to jrafanie/manageiq that referenced this pull request May 8, 2018
@jrafanie
Check all costs fields for relevancy for the report
3c356a0 
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1576101

Manual backport of ManageIQ#17387

If a user asks only for some of the fields (memory cost) in a chargeback report
and they didn't request specific ones (memory allocated cost), we would skip
calculating the memory cost because we thought it wasn't relevant.  We need to
check all cost fields to see if they're relevant for the requested fields in the
report.
@jrafanie jrafanie mentioned this pull request May 8, 2018
Merged
@jrafanie
Copy link
Member Author

jrafanie commented May 8, 2018

Here is the very Fine PR: #17395

@lpichler lpichler mentioned this pull request May 14, 2018
Merged
@simaishi
Copy link
Contributor

Backported to Fine via #17395

simaishi pushed a commit that referenced this pull request May 16, 2018
@gtanzillo @simaishi
Merge pull request #17387 from jrafanie/bring_in_relevant_fields_need…
b89536b 
…ed_for_charge_calculation

Check all costs fields for relevancy for the report
(cherry picked from commit 1588636)

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

Gaprindashvili backport details:

$ git log -1
commit b89536b6567eb9b9493679440b3beae5ca00c252
Author: Gregg Tanzillo <[email protected]>
Date:   Tue May 8 09:27:10 2018 -0400

    Merge pull request #17387 from jrafanie/bring_in_relevant_fields_needed_for_charge_calculation
    
    Check all costs fields for relevancy for the report
    (cherry picked from commit 15886369d79890b9e780a6ff80b084d214f8167b)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1576099

@lpichler lpichler mentioned this pull request May 22, 2018
Merged
d-m-u pushed a commit to d-m-u/manageiq that referenced this pull request Jun 6, 2018
@jrafanie @d-m-u
Check all costs fields for relevancy for the report
27322e0 
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1576101

Manual backport of ManageIQ#17387

If a user asks only for some of the fields (memory cost) in a chargeback report
and they didn't request specific ones (memory allocated cost), we would skip
calculating the memory cost because we thought it wasn't relevant.  We need to
check all cost fields to see if they're relevant for the requested fields in the
report.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpichler lpichler lpichler approved these changes

@gtanzillo gtanzillo gtanzillo approved these changes

Assignees
No one assigned
Labels
blocker bug core/chargeback fine/backported gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jrafanie @miq-bot @lpichler @simaishi @gtanzillo