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

Link recently_discovered_pods widget to rpt #14493

Merged
merged 1 commit into from
Mar 27, 2017
Merged

Link recently_discovered_pods widget to rpt #14493

merged 1 commit into from
Mar 27, 2017
+1 −1

Conversation

kbrock
Copy link
Member

@kbrock kbrock commented Mar 24, 2017 •
edited
Loading

Background

Widgets link to reports by report name.
This is because the ids in our database are not consistent across installs.
There is one widget that references a report by id.

Action

I updated the recently_discovered_pods widget to point to the report with the same name.

MiqReport.find_by(id: 198)
# => nil
MiqReport.find_by(name: "Recently Discovered Pods")
# => <MiqReport .....>

why do you think this is the right report?

  1. There is only one report with a name like "recently discovered pods" (searched on like '%discovered%')
  2. Most widgets have the report name the same or very similar to the widget name.

links

No BZ associated. I found while scanning through widget meta-data

/cc @nimrodshn FYI

@simon3z
Copy link
Contributor

simon3z commented Mar 24, 2017

Thanks @kbrock makes perfect sense.
@nimrodshn did we introduce other of these? (We should fix them).

@kbrock I wonder if we should refuse to seed these in the database if they have an id (explicit ERROR), or if we should have a spec test over these yaml... these would catch future mistakes.

@nimrodshn
Copy link
Contributor

@kbrock Good catch!
@simon3z After re-checking the PR it seems like this was my only buggy reference.

@kbrock
Link recently_discovered_pods widget to rpt
cfded78 
The ids are not consistent in the database across installs
Updated recently_discovered_pods widget to point to the
corresponding report
@kbrock kbrock force-pushed the rpt_recently_discovered_pods branch from 08abb3c to cfded78 Compare March 24, 2017 16:06
@kbrock
Copy link
Member Author

kbrock commented Mar 24, 2017 •
edited
Loading

ugh - bad push. trying to get green (think master is failing)

@nimrodshn to follow up on your comment. All the other reports you created were both good and consistent. So it took seconds to fix. (I'm trying to say thanks)

@miq-bot
Copy link
Member

miq-bot commented Mar 24, 2017

Checked commit kbrock@cfded78 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks good. 🍪

@Fryguy Fryguy merged commit 0d9bdcb into ManageIQ:master Mar 27, 2017
@Fryguy Fryguy added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 27, 2017
@Fryguy
Copy link
Member

Fryguy commented Mar 27, 2017

@kbrock Does this need to go to euwe?

@Fryguy Fryguy assigned Fryguy and unassigned simon3z Mar 27, 2017
@kbrock kbrock deleted the rpt_recently_discovered_pods branch March 27, 2017 16:14
@kbrock
Copy link
Member Author

kbrock commented Mar 27, 2017 •
edited
Loading

This was introduced in 12/6/2016 by #13055
If that is in euwe then we want to get this fix into that version.
UPDATE: No, this file is not in euwe - no need to backport
Note, there is no BZ for this PR (I stumbled upon this while parsing report files)

@kbrock kbrock added euwe/no and removed euwe/yes labels Mar 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Fryguy Fryguy

Labels
bug core/reporting
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kbrock @simon3z @nimrodshn @miq-bot @Fryguy