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

Adding new report and widgets #13055

Merged
merged 1 commit into from
Feb 10, 2017

Conversation

nimrodshn
Copy link
Contributor

@nimrodshn nimrodshn commented Dec 8, 2016

I have added a new report and a new widget as per request of a client.
It is the first widget in a series of widgets suggested by this client in order to support our existing and new container reports.

cc: @simon3z @zeari

Additional Reports:

  • Projects by Number of Containers
  • Pods per Ready Status

Additional Widgets:

  • Pods per Ready widget
  • Pods per Ready chart
  • Nodes by Memory usage
  • Nodes By CPU Usage Widget
  • Recently Discovered Pods Widget
  • Nodes By CPU Capacity
  • Projects By CPU Usage Widget
  • Projects by Number of Pods Widget
  • Projects By Memory Usage Widget
  • Number of Nodes per CPU Cores Chart
  • Projects by Number of Containers

@miq-bot
Copy link
Member

miq-bot commented Dec 8, 2016

@nimrodshn Cannot apply the following label because they are not recognized: widgets

@zeari
Copy link

zeari commented Dec 8, 2016

@miq-bot add_label providers/containers

@zeari
Copy link

zeari commented Dec 8, 2016

@nimrodshn Please verify all the widgets youre adding show correct data and have screenshots of them on this PR.

Edit: Careful when exporting them out of miq since those usually have attributes like user_id\group_id that are specific to the miq instance that exported them.

@chessbyte chessbyte added the wip label Dec 8, 2016
@simon3z
Copy link
Contributor

simon3z commented Dec 12, 2016

@nimrodshn I understand this is a WIP, but there were more widgets to add right?

@nimrodshn
Copy link
Contributor Author

nimrodshn commented Dec 12, 2016

@simon3z yes there are eleven more, I am uploading them carefully one-by-one for three reason:

  1. As @zeari said they contain information which is irrelevant.
  2. Making sure they are working and producing informative results.
  3. The yml the customer produced has six of those widgets combined - thinking ahead - it does not mean every user will want all of those widgets, maybe just some of them.
    It basically comes down to nitpicking in his yml.

@nimrodshn
Copy link
Contributor Author

nimrodshn commented Dec 28, 2016

Added three more widgets as shown in the following screenshot (the three top widgets):
screenshot from 2016-12-28 09-52-26

@nimrodshn
Copy link
Contributor Author

Added three more widgets as noted in the following (top three widgets):
screenshot from 2016-12-29 15-35-55

@@ -0,0 +1,44 @@
---
title: Projects by Number of Containers
Copy link
Contributor

Choose a reason for hiding this comment

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

@nimrodshn please convert this (no CRLF).

@simon3z
Copy link
Contributor

simon3z commented Jan 2, 2017

@nimrodshn anything still missing here? When we'll be able to remove the "WIP"?

@nimrodshn
Copy link
Contributor Author

Six more missing .. (as noted by the checklist above.)

@nimrodshn
Copy link
Contributor Author

nimrodshn commented Jan 4, 2017

Added the rest of widgets needed. @simon3z
screenshot from 2017-01-04 13-13-11

@nimrodshn nimrodshn changed the title [WIP] Adding new report and widgets Adding new report and widgets Jan 4, 2017
@nimrodshn nimrodshn force-pushed the Reports_20161014_095631 branch 7 times, most recently from d46e76b to b756b38 Compare January 5, 2017 11:08
@chessbyte chessbyte removed the wip label Jan 5, 2017
@nimrodshn nimrodshn force-pushed the Reports_20161014_095631 branch 2 times, most recently from c24b7d8 to 613a381 Compare January 11, 2017 10:01
@nimrodshn nimrodshn closed this Jan 11, 2017
@nimrodshn nimrodshn reopened this Jan 11, 2017
@nimrodshn nimrodshn closed this Jan 12, 2017
@nimrodshn nimrodshn reopened this Jan 12, 2017
@simon3z
Copy link
Contributor

simon3z commented Jan 20, 2017

@nimrodshn have you tested all of these?
Once we merge these and release to customers there's little that we can do to update/fix them so they have to be fully correct.

@yaacov
Copy link
Member

yaacov commented Jan 20, 2017

Number of Nodes per Cpu Cores Widget

a. do we need to remind the user it's a widget in the title ... Cores Widget ?
b. the table show nodes per cpu core and not number of nodes per cpu core ?
screenshot-cloud githubusercontent com-2017-01-20-12-46-39

Number of Nodes per Cpu Cores Chart

a. i can see it is a chart ...
b. how useful is this (*) , maybe we should show number of cores per node ?

(*) does it help me to know that i have two machines running 4 cores and one machine running 2 cores ( i do not have information about the cores, and if one core is the same as the other, one can be arm7 and the next a xeon )
screenshot-cloud githubusercontent com-2017-01-20-12-47-22

@nimrodshn
Copy link
Contributor Author

nimrodshn commented Jan 25, 2017

@yaacov all of your remarks are valid.

I just want to be clear - these are widgets that we have received from a client, all I did was to bring them in, clean them and debug them. If I change anything like you have suggested they will not be the same as the ones the client had asked.

Maybe @simon3z can weigh in on these questions?

@simon3z
Copy link
Contributor

simon3z commented Jan 25, 2017

Number of Nodes per Cpu Cores Widget

a. do we need to remind the user it's a widget in the title ... Cores Widget ?

No, it's already in the widget section. Please remove "Widget" from all.

b. the table show nodes per cpu core and not number of nodes per cpu core ?

Actually I don't see any grouping here. I just see a list of nodes with their core numbers.
I don't think this was the intent of the report.

Number of Nodes per Cpu Cores Chart

b. how useful is this (*) , maybe we should show number of cores per node ?

This seems OK as use-case, also you can't display all nodes here (what if you have 300 nodes, why would I care to only display some, and anyway it's impossible to show them all).

@nimrodshn
Copy link
Contributor Author

@simon3z As for you're second comment do you think I should remove 'Number of Nodes per CPU' widget altogether?

@simon3z
Copy link
Contributor

simon3z commented Jan 25, 2017

@simon3z As for you're second comment do you think I should remove 'Number of Nodes per CPU' widget altogether?

@nimrodshn send an email to the author and find out what he intended there (cc our mailing list as well).

@nimrodshn
Copy link
Contributor Author

The author of the widgets have agreed to change the title of that widget, also he had reviewed them and said they look good to him -

screenshot from 2017-01-26 10-26-12
FYI @simon3z @yaacov

@simon3z
Copy link
Contributor

simon3z commented Jan 26, 2017

The author of the widgets have agreed to change the title of that widget, also he had reviewed them and said they look good to him -

@nimrodshn OK but still the name doesn't seem to match the content.

@nimrodshn please squash the patches in one and update the screenshot (if you still have all the data and it's easy, etc.)

@nimrodshn
Copy link
Contributor Author

nimrodshn commented Jan 26, 2017

@simon3z Ah! You are of course correct! - How about this title?
screenshot from 2017-01-26 11-41-37

@nimrodshn nimrodshn force-pushed the Reports_20161014_095631 branch 4 times, most recently from 756f72c to d5a3b3b Compare January 26, 2017 10:26
@nimrodshn
Copy link
Contributor Author

nimrodshn commented Jan 26, 2017

After conversing with @simon3z we have decided to remove 'Nodes By CPU Cores' since It was not informative and we already have a histogram of it.
FYI @yaacov

Added new report

Adding twelve new widgets and a two reports

Added a new report

Added three more Widgets: Nodes by Capacity, Nodes by CPU Usage, Recently Discovered Nodes.

Added six more widgets.

Added a report

Fixed two Reports

Added fixed widget

Added fixes to problems

Fixed small bug in report

Fixed small bugs in Reports

Added small fixes to spec

Fixed CRLF

Added small changes in one of the reports

Fixed some small line in report 130

Removed word widgets from titles

Changes title at 'Number of nodes per CPU cores' widget

Changed the name of 'Number of Nodes per CPU Core' widget

renamed 'number of nodes per cpu' to 'nodes by cpu cores'

Removed 'Nodes by CPU Cores'

Changed spec accordingly
@miq-bot
Copy link
Member

miq-bot commented Jan 26, 2017

Checked commit nimrodshn@7409890 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 🍰

@simon3z
Copy link
Contributor

simon3z commented Jan 26, 2017

@yaacov can you ack if it looks good?

@yaacov
Copy link
Member

yaacov commented Jan 26, 2017

LGTM 👍

@simon3z
Copy link
Contributor

simon3z commented Jan 26, 2017

LGTM 👍 cc @gtanzillo

@miq-bot assign gtanzillo

@miq-bot miq-bot assigned gtanzillo and unassigned simon3z Jan 26, 2017
@simon3z
Copy link
Contributor

simon3z commented Feb 10, 2017

Ping @gtanzillo @lavenel @chessbyte

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 54 Ending Feb 13, 2017 milestone Feb 10, 2017
@gtanzillo gtanzillo merged commit a883256 into ManageIQ:master Feb 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants