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

Convert Container quotas to numeric values #15639

Merged
merged 1 commit into from
Oct 2, 2017
Merged

Convert Container quotas to numeric values #15639

merged 1 commit into from
Oct 2, 2017

Conversation

zakiva
Copy link
Contributor

@zakiva zakiva commented Jul 24, 2017 •
edited
Loading

@zakiva zakiva changed the title Convert Container quotas to numeric values [WIP] Convert Container quotas to numeric values Jul 24, 2017
@zakiva
Copy link
Contributor Author

zakiva commented Jul 24, 2017

@simon3z @zeari

@zakiva
Copy link
Contributor Author

zakiva commented Jul 24, 2017

@miq-bot add_label providers/containers, enhancement

moolitayer
moolitayer reviewed Jul 24, 2017
View reviewed changes
app/models/container_quota_item.rb Outdated
@@ -1,3 +1,15 @@
class ContainerQuotaItem < ApplicationRecord
belongs_to :container_quota

def quota_desired
quota_desired_int || quota_desired_float
Copy link

Choose a reason for hiding this comment

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

@zakiva if these columns are always equal do we really need two columns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

one of them is always nil while the other contains the actual quota value (which can be either int or float)

@bronaghs
Copy link

@miq-bot assign @blomquisg

@miq-bot miq-bot assigned blomquisg and unassigned bronaghs Jul 24, 2017
@zakiva zakiva mentioned this pull request Jul 26, 2017
Merged
@zakiva zakiva mentioned this pull request Aug 9, 2017
Merged
@zakiva zakiva changed the title [WIP] Convert Container quotas to numeric values Convert Container quotas to numeric values Aug 9, 2017
@zakiva
Copy link
Contributor Author

zakiva commented Aug 9, 2017 •
edited
Loading

@miq-bot rm_label wip

@miq-bot miq-bot removed the wip label Aug 9, 2017
@zakiva zakiva mentioned this pull request Sep 4, 2017
Merged
simon3z
simon3z reviewed Sep 6, 2017
View reviewed changes
app/models/container_quota_item.rb Outdated
private

def quota_display(quota)
val = quota == quota.to_i ? quota.to_i : quota
Copy link
Contributor

Choose a reason for hiding this comment

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

@zakiva why do we need this?

Copy link
Contributor Author

@zakiva zakiva Sep 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I want to get rid of the .0 when displaying integers values in the UI, especially for counts of objects it will be strange to see a count of say 5.0 pods instead of just 5 pods

Copy link
Contributor

Choose a reason for hiding this comment

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

@zakiva I suppose this is can be achieved in a cleaner way with:

def quota_display(quota)
    (quota % 1 == 0) ? quota.to_i.to_s : quota.to_s
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nice, I will change that

@blomquisg
Copy link
Member

@simon3z you good with this? If so, I got a green button here.

@simon3z
Copy link
Contributor

simon3z commented Sep 18, 2017

@simon3z you good with this? If so, I got a green button here.

@blomquisg I think there's some code that can be simplified (see comment above).
Once ready let's remember that we have some dependencies to merge as well.

@blomquisg
Copy link
Member

let's remember that we have some dependencies to merge as well.

Heh, good thing you remembered! I'm going to wip this until the schema is merged just so I don't get trigger happy.

@blomquisg blomquisg added the wip label Sep 18, 2017
@simon3z
Copy link
Contributor

simon3z commented Sep 19, 2017

@zakiva I think quota_display deserves a test to make sure output is correct in all edge cases.

@zakiva zakiva force-pushed the quota_numeric_values branch 2 times, most recently from 841fdd6 to 140cddc Compare September 19, 2017 11:58
simon3z
simon3z approved these changes Sep 19, 2017
View reviewed changes
Copy link
Contributor

@simon3z simon3z left a comment

Choose a reason for hiding this comment

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

LGTM 👍
cc @blomquisg

@Fryguy
Copy link
Member

Fryguy commented Sep 28, 2017

Merged the schema, so bouncing this PR.

@Fryguy Fryguy closed this Sep 28, 2017
@Fryguy Fryguy reopened this Sep 28, 2017
@zakiva
Copy link
Contributor Author

zakiva commented Sep 29, 2017

@miq-bot remove_label wip
cc @blomquisg @Fryguy

@miq-bot miq-bot removed the wip label Sep 29, 2017
@miq-bot
Copy link
Member

miq-bot commented Oct 1, 2017

Checked commit zakiva@22f10df with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@blomquisg blomquisg merged commit 61b9dfb into ManageIQ:master Oct 2, 2017
@blomquisg blomquisg added this to the Sprint 70 Ending Oct 2, 2017 milestone Oct 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simon3z simon3z simon3z approved these changes

@moolitayer moolitayer moolitayer approved these changes

Assignees

@blomquisg blomquisg

Labels
enhancement providers/containers
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@zakiva @bronaghs @blomquisg @simon3z @Fryguy @miq-bot @moolitayer