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

Changed report name to be consistent with actual produced report. #14646

Merged
merged 1 commit into from
May 23, 2017

Conversation

nimrodshn
Copy link
Contributor

Fixed report name to be consistent with report produced, also fixed OOTB chart based on that report.
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1436706
Before:
screenshot from 2017-04-05 11-14-39
After:
screenshot from 2017-04-05 11-19-20
Chart (Top right one):
screenshot from 2017-04-05 11-20-15

cc: @simon3z @moolitayer @yaacov

@yaacov
Copy link
Member

yaacov commented Apr 5, 2017

a. In the BZ it say the currect title of the report table should be "Nodes by Number of CPU Cores"
not "Nodes per CPU Cores"

b. The report chart on the other hand show "number of nodes per cpu cores"

@chessbyte chessbyte changed the title Changed report name to be consistent with actoual produced report. Changed report name to be consistent with actual produced report. Apr 5, 2017
@nimrodshn nimrodshn force-pushed the fix_report_name_nodes_per_cpu branch from ddde839 to 3f8a4b6 Compare April 5, 2017 09:03
@nimrodshn
Copy link
Contributor Author

nimrodshn commented Apr 5, 2017

a. In the BZ it say the currect title of the report table should be "Nodes by Number of CPU Cores"
not "Nodes per CPU Cores"

Fixed.

b. The report chart on the other hand show "number of nodes per cpu cores"

from what I understanf its OKAY for a chart to show something other than the report it grabs information from - i.e. a chart grabs information from a report in order to show some info; It isn't an exact copy of that report otherwise there is no use for charts - just use reports instead.

Meaning the term 'report chart' is empty - charts draw information from reports but are different entities from them. [thats how I think of this..]

@nimrodshn
Copy link
Contributor Author

screenshot from 2017-04-05 12-01-42

@miq-bot
Copy link
Member

miq-bot commented Apr 5, 2017

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

@yaacov
Copy link
Member

yaacov commented Apr 5, 2017

LGTM 👍

Meaning the term 'report chart' is empty - charts draw information from reports but are different entities from them. [thats how I think of this..]

A chart should have a title .... but that is a different bug :-)

@yaacov
Copy link
Member

yaacov commented Apr 5, 2017

p.s.

The chart doesn't have a title or a legend !? so it's a mystery chart :-)

@nimrodshn
Copy link
Contributor Author

The chart doesn't have a title or a legend !? so it's a mystery chart :-)

a. It does have a title.
b. As far as I know we do not support legends...

@simon3z
Copy link
Contributor

simon3z commented Apr 11, 2017

@nimrodshn @yaacov since there are tens or hundreds of nodes displaying them in a table ordered by CPU cores is not relevant.
The idea was to have a table/chart that is displaying the number of nodes by CPU cores:

Number of Nodes Number of CPU Cores
4 8
32 4

Of course the best output for this is the chart based on this data.

Please check #3220 when I introduced it (see how a field was named "Number of Nodes per CPU Cores"). Did something change in the reporting system? Is it possible to have the report as it was initially designed?

@nimrodshn
Copy link
Contributor Author

@zeari you're opinion?

@zeari
Copy link

zeari commented May 8, 2017

@nimrodshn I dont know of a way to leverage the reports generator to do this.
what we want here is to group by 'Number of CPU Cores' and get counts for each. I think the best we can do is get counts through the consolidation tab in the reports editor.

@nimrodshn
Copy link
Contributor Author

@simon3z I'm not sure we can produce the report you described as now, also looking at the original report it doesn't seem like it had looked like this to begin with... Thoughts?

@simon3z
Copy link
Contributor

simon3z commented May 12, 2017

LGTM 👍
@gtanzillo @nimrodshn @zeari @kbrock in an already existing system are these going to be duplicated because we changed the name?
@miq-bot assign gtanzillo

@miq-bot miq-bot assigned gtanzillo and unassigned simon3z May 12, 2017
@nimrodshn
Copy link
Contributor Author

@simon3z looking at my app there aren't any dups.

@kbrock
Copy link
Member

kbrock commented May 18, 2017

Not sure what will happen to entries in MiqQueue that point to the old widget names.
To be honest, this should be fine.

Other than that, this looks great

@nimrodshn
Copy link
Contributor Author

@simon3z FYI.

@simon3z
Copy link
Contributor

simon3z commented May 23, 2017

@simon3z FYI.

@nimrodshn the PR is assigned to @gtanzillo (ping?)

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 62 Ending Jun 5, 2017 milestone May 23, 2017
@gtanzillo gtanzillo merged commit 5e34877 into ManageIQ:master May 23, 2017
@simaishi
Copy link
Contributor

@gtanzillo @simon3z should this be fine/yes (as per BZ)?

@gtanzillo
Copy link
Member

👍 fine/yes

simaishi pushed a commit that referenced this pull request Jun 14, 2017
Changed report name to be consistent with actual produced report.
(cherry picked from commit 5e34877)

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

Fine backport details:

$ git log -1
commit c5cc90db5db590cbee648c6917aa3008d6d77940
Author: Gregg Tanzillo <[email protected]>
Date:   Tue May 23 13:56:49 2017 -0400

    Merge pull request #14646 from nimrodshn/fix_report_name_nodes_per_cpu
    
    Changed report name to be consistent with actual produced report.
    (cherry picked from commit 5e34877484eb497ba30dadd47bcbc47860192db0)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1461541

@simaishi
Copy link
Contributor

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.

8 participants