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

Fix Topology graph for Physical Infra Providers #656

Merged
merged 1 commit into from
Apr 5, 2017
Merged

Fix Topology graph for Physical Infra Providers #656

merged 1 commit into from
Apr 5, 2017

Conversation

AparnaKarve
Copy link
Contributor

screen shot 2017-03-10 at 10 03 32 am

@himdel This is a Topology follow-up PR for #196

@dclarizio dclarizio requested a review from himdel March 11, 2017 00:33
@dclarizio dclarizio self-assigned this Mar 11, 2017
himdel
himdel reviewed Mar 13, 2017
View reviewed changes
app/views/physical_infra_topology/show.html.haml
%circle{:r => "17"}
-# pficon-cloud-tenant
%text{:y => "9"} 
-# pficon-physical_servers
Copy link
Contributor

Choose a reason for hiding this comment

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

pficon-physical_servers or product-physical_server? (https://github.com/ManageIQ/manageiq-ui-classic/pull/656/files#diff-94458330f9392b3aab4f5e1c7fed68c9R32)

Copy link
Contributor

Choose a reason for hiding this comment

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

Aaah.. pficon-server-group :)

Copy link
Contributor

Choose a reason for hiding this comment

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

=> #922

@himdel
Copy link
Contributor

himdel commented Mar 13, 2017

Double-clicking the provider gives me a 404 on /physical_infra_manager/show/10000000000038 (guessing it should be /ems_physical_infra/10r38, which works).

@himdel
Copy link
Contributor

himdel commented Mar 13, 2017

Maybe add Topology to the menu as well?

@AparnaKarve
Copy link
Contributor Author

Maybe add Topology to the menu as well?

This change would be in core MIQ.

Can we get this classic UI specific fix merged today?

@himdel
Copy link
Contributor

himdel commented Mar 27, 2017

This change would be in core MIQ.

app/presenters/menu/default_menu.rb lives in manageiq-ui-classic ;)

@himdel
Copy link
Contributor

himdel commented Mar 27, 2017

..but feel free to add it to the menu in a separate PR :)

But, there's still the double click issue..

@AparnaKarve AparnaKarve force-pushed the fix_physical_infra_topology branch from 13b9302 to 465ae8c Compare March 30, 2017 23:57
@miq-bot
Copy link
Member

miq-bot commented Mar 30, 2017

Checked commit AparnaKarve@465ae8c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks good. 🏆

@AparnaKarve
Copy link
Contributor Author

app/presenters/menu/default_menu.rb lives in manageiq-ui-classic ;)

True, but not the product feature id required for it ;)
Created #862 and ManageIQ/manageiq#14589

@AparnaKarve
Copy link
Contributor Author

Also fixed the double-click issue on the Physical Infra Provider.

Note that for the Physical Server double-click to work, we would need #755 merged.

IMO, this PR can now be merged after you have verified that Physical Infra Provider double-click is fixed.

@AparnaKarve AparnaKarve reopened this Mar 31, 2017
@himdel
Copy link
Contributor

himdel commented Apr 5, 2017

Thanks @AparnaKarve fixes topology nicely, no sense in waiting for that remaining icon name issue, merging.

@himdel himdel merged commit ecc60b7 into ManageIQ:master Apr 5, 2017
@himdel himdel added this to the Sprint 58 Ending Apr 10, 2017 milestone Apr 5, 2017
@himdel himdel mentioned this pull request Apr 5, 2017
Merged
simaishi pushed a commit that referenced this pull request Apr 5, 2017
@himdel @simaishi
Merge pull request #656 from AparnaKarve/fix_physical_infra_topology
3c36649 
Fix Topology graph for Physical Infra Providers
(cherry picked from commit ecc60b7)
@simaishi
Copy link
Contributor

simaishi commented Apr 5, 2017

Fine backport details:

$ git log -1
commit 3c36649c8b24737b8ec6a28da476fcb3f50d62d3
Author: Martin Hradil <[email protected]>
Date:   Wed Apr 5 09:43:14 2017 +0000

    Merge pull request #656 from AparnaKarve/fix_physical_infra_topology
    
    Fix Topology graph for Physical Infra Providers
    (cherry picked from commit ecc60b7f609d95d7989354af9b1b5507ef99c10a)

@AparnaKarve AparnaKarve deleted the fix_physical_infra_topology branch July 26, 2017 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@himdel himdel himdel left review comments

Assignees

@dclarizio dclarizio

Labels
bug compute/physical infrastructure fine/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@AparnaKarve @himdel @miq-bot @simaishi @dclarizio