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

Add migration analytics reports #18749

Merged
merged 5 commits into from
May 23, 2019
Merged

Add migration analytics reports #18749

merged 5 commits into from
May 23, 2019

Conversation

pemcg
Copy link

@pemcg pemcg commented May 9, 2019

This PR provides the reports for https://bugzilla.redhat.com/show_bug.cgi?id=1708369

The 2 new reports are:
Migration Readiness/Virtual Machines/VMware VM Summary
Migration Readiness/Providers/VMware Environment Summary

@pemcg
Copy link
Author

pemcg commented May 9, 2019

@miq-bot add_label hammer:yes

@pemcg
Copy link
Author

pemcg commented May 9, 2019

@miq-bot add_label Enhancement

@pemcg
Copy link
Author

pemcg commented May 9, 2019

@miq-bot add_label z-stream

@miq-bot
Copy link
Member

miq-bot commented May 9, 2019

@pemcg Cannot apply the following label because they are not recognized: hammer:yes

@miq-bot
Copy link
Member

miq-bot commented May 9, 2019

@pemcg Cannot apply the following label because they are not recognized: z-stream

@pemcg
Copy link
Author

pemcg commented May 9, 2019

@miq-bot add_label hammer/yes

@pemcg
Copy link
Author

pemcg commented May 9, 2019

@miq-bot add_label changelog/yes

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

looks good. just the minor thoughtz

@martinpovolny who tends to review these "ui" PRs?

value: ''
- "=":
field: Vm-type
value: ManageIQ::Providers::Vmware::InfraManager::Vm
Copy link
Member

Choose a reason for hiding this comment

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

would it make sense to just do:

db: ManageIQ::Providers::Vmware::InfraManager::Vm

Copy link
Author

Choose a reason for hiding this comment

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

This db object wouldn't be available for a report created using the UI, the only option in the "Base the report on" drop-down is "Virtual Machines". Happy to change, but would that be subsequently copy/editable in the UI at a later stage by a user?

Copy link
Member

Choose a reason for hiding this comment

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

sigh. it is important to be editable/hackable.

For this one keeping it this way would probably be the same

Copy link
Member

Choose a reason for hiding this comment

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

Yea, keep this

exp:
"=":
field: Host.ext_management_system-type
value: ManageIQ::Providers::Vmware::InfraManager
Copy link
Member

Choose a reason for hiding this comment

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

think using a Vmware field for the db: host would also be more efficient

Copy link
Author

Choose a reason for hiding this comment

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

as above

Copy link
Member

Choose a reason for hiding this comment

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

is it possible to put a test on Host-type instead of the join into ems? (not sure if that actually exists)

rpt_type: Custom
priority: 10
db: Host
cols:
Copy link
Member

Choose a reason for hiding this comment

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

@martinpovolny are we creating reports with cols and include or are we starting to do reports without these optional columns?

Copy link
Member

Choose a reason for hiding this comment

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

@kbrock : shall we modify the report editor to skip these fields?

Copy link
Member

Choose a reason for hiding this comment

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

@martinpovolny nah - probably too much work.

Keep these columns in.

db_options: {}
generate_cols:
generate_rows:
col_formats:
Copy link
Member

Choose a reason for hiding this comment

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

also, do we like specifying all these columns to make it easier to copy/paste to a new report, or do we remove fields that are no longer used?

Copy link
Member

Choose a reason for hiding this comment

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

I guess we should have modified the report editor after you did the changes that made these fields redundant.

@miq-bot
Copy link
Member

miq-bot commented May 10, 2019

Checked commits pemcg/manageiq@31539e8~...ddde500 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

Copy link
Member

@kbrock kbrock left a comment

Choose a reason for hiding this comment

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

LGTM

@aufi
Copy link
Member

aufi commented May 21, 2019

👍

@Fryguy Fryguy merged commit 21ec615 into ManageIQ:master May 23, 2019
@Fryguy Fryguy added this to the Sprint 112 Ending May 27, 2019 milestone May 23, 2019
simaishi pushed a commit that referenced this pull request Jun 10, 2019
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit b655f244f3168edcf4e6663611c745c5604cd35b
Author: Jason Frey <jfrey@redhat.com>
Date:   Thu May 23 11:44:56 2019 -0400

    Merge pull request #18749 from pemcg/add_migration_analytics_reports
    
    Add migration analytics reports
    
    (cherry picked from commit 21ec615ae062ed4d692cdcbb4b91a885bc5e2bab)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1718916

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