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

Add an 'Allocated cpu cores' chargeback rate detail to existing chargeback rates #108

Merged
merged 1 commit into from
Oct 30, 2017

Conversation

zeari
Copy link

@zeari zeari commented Oct 26, 2017

Adds the rate detail used in ManageIQ/manageiq#15616 to existing chargeback rates
@lpichler Please review
cc @moolitayer @cben @simon3z

@@ -0,0 +1,27 @@
class AddCoresAllocatedRateDetail < ActiveRecord::Migration[5.0]
def up
Copy link
Contributor

Choose a reason for hiding this comment

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

@zeari

need to ad stub

  class ChargeableField < ActiveRecord::Base
  end

@zeari zeari force-pushed the add_rate_detail branch 4 times, most recently from c47de66 to 51360c8 Compare October 26, 2017 14:34
@Fryguy
Copy link
Member

Fryguy commented Oct 26, 2017

Doesn't seeding handle this? @lpichler ?

@lpichler
Copy link
Contributor

@Fryguy no, this is needed for existing ChargebackRates. Seeding will just provide ability to use this new metric when new ChargebackRate is added.

@chessbyte chessbyte added the data label Oct 27, 2017
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.

👍

cc @gtanzillo


migrate

expect(default_rate.chargeback_rate_details.where(:description => "Allocated CPU Cores").count).to eq(1)
Copy link
Member

Choose a reason for hiding this comment

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

Since the default_rate object is memoized from the call on line 25, can you change this to a default_rate.reload.chargeback_rate_details...? This way we can ensure it's getting fresh data.

migrate

expect(default_rate.chargeback_rate_details.where(:description => "Allocated CPU Cores").count).to eq(1)
expect(custom_rate.chargeback_rate_details.where(:description => "Allocated CPU Cores").count).to eq(1)
Copy link
Member

Choose a reason for hiding this comment

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

Same

@zeari
Copy link
Author

zeari commented Oct 29, 2017

Since the default_rate object is memoized from the call on line 25, can you change this to a default_rate.reload.chargeback_rate_details...? This way we can ensure it's getting fresh data.

Done @Fryguy

@miq-bot
Copy link
Member

miq-bot commented Oct 29, 2017

Checked commit zeari@008367e with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. ⭐

@Fryguy Fryguy merged commit 80434c3 into ManageIQ:master Oct 30, 2017
@Fryguy Fryguy added this to the Sprint 72 Ending Oct 30, 2017 milestone Oct 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants