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

Removed duplicate report #14515

Merged
merged 1 commit into from
Apr 11, 2017
Merged

Removed duplicate report #14515

merged 1 commit into from
Apr 11, 2017

Conversation

nimrodshn
Copy link
Contributor

Due to #14494 I have removed the the duplicate report and renamed the old one for consistency with widgets.

cc @kbrock @simon3z @moolitayer

removed dup reports
@miq-bot
Copy link
Member

miq-bot commented Mar 27, 2017

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

@nimrodshn
Copy link
Contributor Author

I have tested widgets and report to see changes did not affect them:
Report:
screenshot from 2017-03-27 12-06-30
Widgets (top two):
screenshot from 2017-03-27 12-23-53

Everything seems to work fine (to be expected..)

@nimrodshn
Copy link
Contributor Author

@miq-bot add_label providers/containers reporting

@miq-bot
Copy link
Member

miq-bot commented Mar 27, 2017

@nimrodshn Cannot apply the following label because they are not recognized: providers/containers reporting

@nimrodshn
Copy link
Contributor Author

and again - thanks to @kbrock for the catch!

@moolitayer
Copy link

moolitayer commented Mar 27, 2017

@nimrodshn what is the meaning of

miq_group_id: 1
user_id: 1

asking since they exists only in the report the remains

Also this exists only in the file you removed, not sure what it does:

filename: 170_Configuration Management - Containers/060_Container Groups per Ready Status.yaml

@moolitayer
Copy link

@miq-bot add_label bug, providers/containers, reporting
fix #14494

@nimrodshn
Copy link
Contributor Author

nimrodshn commented Mar 27, 2017

@moolitayer I think it has something to do with who is allowed to use this report.. to be quite honest I'm not sure as I did not write this report - Although from previous discussion I had (I think with @zeari) I think it can be removed. Also note they exist in other reports (say 020_Nodes by CPU Usage.yaml).

As for your'e other question the line you are pointing to is exactly the reason we are deleting that report - it turns out it was a duplicate of the one I renamed - moreover it didn't do anything but draw information from that one so I just deleted it and renamed the old one as to be consistent with other widgets I had brought in.

@zeari
Copy link

zeari commented Mar 28, 2017

@moolitayer I think it has something to do with who is allowed to use this report.. to be quite honest I'm not sure as I did not write this report - Although from previous discussion I had (I think with @zeari) I think it can be removed. Also note they exist in other reports (say 020_Nodes by CPU Usage.yaml).

AFAIK, miq_group_id and user_id shouldnt be in 'out of the box' reports.

@nimrodshn LGTM (im counting on you that none of the related widgets\reports reports fail)

@moolitayer
Copy link

I'm in favor of going for the other solution @kbrock suggested in #14494: renaming the duplicate title. Maybe also comment. The reason is that old scheduled reports and widgets would fail. @kbrock @simon3z WDYT?

@kbrock
Copy link
Member

kbrock commented Mar 29, 2017

@moolitayer they look like duplicates to me.
Not sure what you would do to change the title for 2 identical reports

@nimrodshn
Copy link
Contributor Author

nimrodshn commented Mar 30, 2017

@kbrock I think what @moolitayer is saying is that once I delete one of the reports (either the dup or the original) users who are using it as base for custom widgets will lose their widgets since they are based on a report which exist no more. Although, renaming one of the reports is the same as removing it - So I don't see how this gets around this.. what do you think?

@kbrock
Copy link
Member

kbrock commented Mar 31, 2017

@nimrodshn Do custom widgets to reference reports by name? If so, then there shouldn't be a problem.

If not, then can we write a migration?
To be honest, since reports are unique by name, I'm not sure if we will have to go into the db and manually delete one of these reports.

@nimrodshn
Copy link
Contributor Author

@kbrock Custom widgets (as do "out of the box" widgets) reference reports by name.

@kbrock
Copy link
Member

kbrock commented Apr 7, 2017

@nimrodshn Phew. I was concerned that I was totally off here.

@kbrock I think what @moolitayer is saying is that once I delete one of the reports (either the dup or the original) users who are using it as base for custom widgets will lose their widgets since they are based on a report which exist no more. Although, renaming one of the reports is the same as removing it - So I don't see how this gets around this.. what do you think?

Please do not rename.

The original and custom widgets refer to a report by name.
We don't know which report will be returned. Luckily both reports have the same data.
After you delete this file, the widgets system refer to a single report with this name.
So nothing will break.
It will actually be more stable.

@moolitayer
Copy link

@kbrock @nimrodshn sorry for holding this back. Just to be sure

  • what about scheduled reports? do we need to migrate that?
  • when you say widgets refer to reports by name do you mean menu_name, title or filename?

@nimrodshn
Copy link
Contributor Author

nimrodshn commented Apr 9, 2017

@moolitayer

when you say widgets refer to reports by name do you mean menu_name, title or filename?

I mean resource_name.
So, if you look at 'Pods per Ready' chart/widget (at product/dashboard/widgets) you will find both are "pointing to" the Pods per Ready report - the Duplicate - that is why I had changed the original name to that - So that I will be able to produce those widgets.

@kbrock if I take you're route I need to change the OOTB widgets ('Pods per Ready' chart/widget) to point to the duplicate.. are you okay with this?

@moolitayer
Copy link

I mean resource_name.
So, if you look at 'Pods per Ready' chart/widget (at product/dashboard/widgets) you will find both are "pointing to" the Pods per Ready report - the Duplicate - that is why I had changed the original name to that - So that I will be able to produce those widgets.

Oh so resource_name points to the reports' title

@nimrodshn
Copy link
Contributor Author

@moolitayer Precisely!

@kbrock kbrock merged commit d37a591 into ManageIQ:master Apr 11, 2017
@kbrock kbrock added this to the Sprint 59 Ending Apr 24, 2017 milestone Apr 11, 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