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

Add Report: Projects by Quota Items #15776

Merged
merged 1 commit into from
Oct 11, 2017
Merged

Add Report: Projects by Quota Items #15776

merged 1 commit into from
Oct 11, 2017

Conversation

zakiva
Copy link
Contributor

@zakiva zakiva commented Aug 9, 2017 •
edited
Loading

@zakiva
Copy link
Contributor Author

zakiva commented Aug 9, 2017

@miq-bot add_label providers/containers, reporting

@zakiva
Copy link
Contributor Author

zakiva commented Aug 9, 2017

@simon3z @zeari Please review

@simon3z
Copy link
Contributor

simon3z commented Aug 9, 2017

@zakiva @zeari IIRC you can have multiple quota definitions per project. How do you handle that case? I was expecting somewhere the code to do the union between all definitions. Is it somewhere else (or have I missed it?)?

@simon3z
Copy link
Contributor

simon3z commented Aug 9, 2017

@zakiva @zeari IIRC you can have multiple quota definitions per project. How do you handle that case? I was expecting somewhere the code to do the union between all definitions. Is it somewhere else (or have I missed it?)?

@zakiva @zeari also WRT to the OOTB report, can we have the name of the quota object as well as a column?

@zeari
Copy link

zeari commented Aug 9, 2017 •
edited
Loading

@zakiva @zeari also WRT to the OOTB report, can we have the name of the quota object as well as a column?

we can probably get ContainerQuota.name in each row

@zakiva @zeari IIRC you can have multiple quota definitions per project. How do you handle that case? I was expecting somewhere the code to do the union between all definitions. Is it somewhere else (or have I missed it?)?

It doesnt unionize. I think we should show each quota item with the name of the ContainerQuota it belongs to. It would reflect more accurately whats happening on the provider side.

@zakiva
Copy link
Contributor Author

zakiva commented Aug 13, 2017 •
edited
Loading

@simon3z @zeari I added another column for the quota name and updated the screenshot above, please take a look.

zeari
zeari reviewed Aug 13, 2017
View reviewed changes
spec/models/miq_report/seeding_spec.rb
@@ -1,5 +1,5 @@
describe MiqReport do
context "Seeding" do
include_examples(".seed called multiple times", 144)
include_examples(".seed called multiple times", 145)
end
end
Copy link

@zeari zeari Aug 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Ugh, again with these counting specs...
where does it end

Copy link
Member

Choose a reason for hiding this comment

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

You 👏 can 👏 never 👏 stop 👏 counting!!

@zeari
Copy link

zeari commented Aug 13, 2017

I was expecting somewhere the code to do the union between all definitions.

@zakiva Can investigate what happens on the OSE side when you define several quotas that restrict the same resource? (does it take the minimum? ignore one of them?)

@zakiva
Copy link
Contributor Author

zakiva commented Aug 15, 2017

@zakiva Can investigate what happens on the OSE side when you define several quotas that restrict the same resource? (does it take the minimum? ignore one of them?)

@zeari In such case the minimum value will apply.

@zeari
Copy link

zeari commented Aug 17, 2017

@zeari In such case the minimum value will apply.

Ok so i dont think we should unionize or change the data in any way. This should reflect exactly whats happening on the provider side. A good change here would be to sort by project and then by resource.
It would be more noticable that the same project defined the same resource constraint twice or more.

@zakiva
Copy link
Contributor Author

zakiva commented Aug 20, 2017

Ok so i dont think we should unionize or change the data in any way. This should reflect exactly whats happening on the provider side. A good change here would be to sort by project and then by resource.
It would be more noticable that the same project defined the same resource constraint twice or more.

@zeari good idea 👍 done

@simon3z
Copy link
Contributor

simon3z commented Aug 24, 2017

@zakiva @zeari the report LGTM 👍

@miq-bot
Copy link
Member

miq-bot commented Sep 29, 2017

This pull request is not mergeable. Please rebase and repush.

@simon3z
Copy link
Contributor

simon3z commented Oct 3, 2017

@miq-bot assign blomquisg
cc @blomquisg

@miq-bot miq-bot assigned blomquisg and unassigned simon3z Oct 3, 2017
blomquisg
blomquisg suggested changes Oct 5, 2017
View reviewed changes
Copy link
Member

@blomquisg blomquisg left a comment

Choose a reason for hiding this comment

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

Can you check on whether there's an extra commit or something in this PR? There's a change in this PR that's already in master. And, yet, I don't see that it's a merge conflict.

app/models/container_quota_item.rb Outdated

def quota_display(quota)
(quota % 1).zero? ? quota.to_i.to_s : quota.to_s
end
Copy link
Member

Choose a reason for hiding this comment

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

I think these changes were part of another PR? These changes are already in master.

I can't figure out why this is not giving a merge conflict...

@miq-bot
Copy link
Member

miq-bot commented Oct 9, 2017

Checked commit zakiva@a74ac6e with ruby 2.3.3, rubocop 0.47.1, and haml-lint 0.20.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@zakiva
Copy link
Contributor Author

zakiva commented Oct 9, 2017

@blomquisg Rebased, PTAL

@blomquisg blomquisg merged commit 38a690b into ManageIQ:master Oct 11, 2017
@blomquisg blomquisg added this to the Sprint 71 Ending Oct 16, 2017 milestone Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zeari zeari zeari left review comments

@simon3z simon3z simon3z approved these changes

@blomquisg blomquisg blomquisg approved these changes

Assignees

@blomquisg blomquisg

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

Successfully merging this pull request may close these issues.
6 participants
@zakiva @simon3z @zeari @miq-bot @blomquisg @chessbyte