Topology for Container Projects #120

zeari · 2017-01-10T15:41:26Z

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1426229
From projects summary page click on the topology view toggle button:

Land here:

@martinpovolny PTAL, I remember you refactored topology recently

ContainerProjectTopologyService is ContainerTopologyService with a different build_topology method but i can throw the shared code into a mixin. I also couldnt get around creating ContainerProjectTopologyController which is more of the same.
we should probably have a system in toolbar_chooser to figure out which entities have topology\dashboard views and show the upper right toolbar icons accordingly. Im guessing a role_allows?(:feature => 'topologyview')

cc @simon3z
@miq-bot add_label topology, providers/containers, ui, wip

miq-bot · 2017-01-10T15:41:37Z

@zeari Cannot apply the following labels because they are not recognized: topology, ui

martinpovolny · 2017-01-27T09:51:10Z

Overall it looks good!

I don't get how the part entities have topology\dashboard views relates to role_allows??

I see it more related to capabilities and the supports? framework that we have on models. But this is really not a capability or feature of the backed component but rather a property of the UI related to the models.

We may use the decorators for this -- the decarators on the classes (not instances) that @himdel is just adding.

I am concerned about entity_display_type and entity_status -- those look pretty ugly, probably again a use case for some decorators or presenter classes. Also I wonder how i18n is (is not?) handled for the strings used there.

zeari · 2017-02-02T15:42:06Z

app/helpers/application_helper/toolbar_chooser.rb

@@ -43,6 +43,8 @@ def view_toolbar_filename
      'drift_view_tb'
    elsif %w(ems_container ems_infra).include?(@layout) && %w(main dashboard topology).include?(@display)
      'dashboard_summary_toggle_view_tb'
+    elsif %w(container_project).include?(@layout)
+      "#{@layout}_view_tb"


I don't get how the part entities have topology\dashboard views relates to role_allows??

I thought that if an entity (like ems) has a dashboard view available (maybe through supports?) then we should add the toolbar dashboard view button instead of picking specific entities with their toolbars.
or maybe we always have entities search for their "#{@layout}_view_tb" file.
Im just trying to prevent massive refactors later...

@martinpovolny

sorry, I don't get it :-(

zeari · 2017-02-02T15:44:01Z

@dclarizio @serenamarie125 Can I get a general ack towards making this change?

serenamarie125 · 2017-02-02T16:51:11Z

@zeari is this essentially adding the Topology icon in the view selector ?

zeari · 2017-02-02T17:16:36Z

@zeari is this essentially adding the Topology icon in the view selector ?

@serenamarie125 that goes to a project level topology screen, yes.

serenamarie125

This is exactly what UX designed and recommends. Thanks @zeari

miq-bot · 2017-02-10T19:27:48Z

This pull request is not mergeable. Please rebase and repush.

zeari · 2017-02-22T15:12:53Z

@miq-bot remove_label wip

miq-bot · 2017-02-22T15:13:23Z

@zeari Cannot apply the following labels because they are not recognized: topology, providers/containers

zeari · 2017-02-22T15:25:17Z

@martinpovolny @himdel PTAL

zeari · 2017-02-22T21:54:19Z

I am concerned about entity_display_type and entity_status -- those look pretty ugly, probably again a use case for some decorators or presenter classes. Also I wonder how i18n is (is not?) handled for the strings used there.

Im not sure whats the next step here. right now i just refactored that code into the mixin.

simon3z · 2017-02-23T09:43:29Z

I am concerned about entity_display_type and entity_status -- those look pretty ugly, probably again a use case for some decorators or presenter classes. Also I wonder how i18n is (is not?) handled for the strings used there.

Im not sure whats the next step here. right now i just refactored that code into the mixin.

@martinpovolny @zeari those methods are definitely ugly and must be improved, but if this PR is about just moving some code from one place to another (mixin) I don't think we should block on a technical debt / refactoring.

zeari · 2017-02-23T10:19:40Z

app/services/container_project_topology_service.rb

+                                                      :ContainerServices   => { :ContainerRoutes => nil }
+                                                    }
+                                                  }
+                           }


@simon3z Do we want to show nodes\Vms in this topology?

@zeari are we taking advantage of the same logic of auto-disabling entities when we reach a limit? (PR #95) I think we should, because indeed we would like to show nodes and VMs here.

@zeari are we taking advantage of the same logic of auto-disabling entities when we reach a limit? (PR #95) I think we should, because indeed we would like to show nodes and VMs here.

Yes. Im adding nodes and vms.

simon3z · 2017-02-24T10:59:56Z

app/assets/javascripts/controllers/container_topology/container_topology_controller.js

    } else {
      // search for pattern ^/<controler>/<id>$ in the pathname
+      type = 'container_topology'


@zeari @martinpovolny this logic of this functions seems to become a little complex to maintain. Maybe we don't have to keep the old container_topology path if it's becoming an exception (as you need an extra else branch).

Correct me if I'm wrong please, but .. isn't it always container_topology here? We don't really reuse the controller for any other topologies now, do we?

Correct me if I'm wrong please, but .. isn't it always container_topology here? We don't really reuse the controller for any other topologies now, do we?

in this case it could also be container_project_topology. container_topology would lead to code from ContainerTopologyService which generates topology for ems and not projects.

@zeari @martinpovolny this logic of this functions seems to become a little complex to maintain. Maybe we don't have to keep the old container_topology path if it's becoming an exception (as you need an extra else branch).

in that case we would need the /ems_container/ path to build topology for ems .
right now there are 3 cases for the url:

all ems topology (the path/container_topology/ without an id)

single ems topology (the same but with id - /ems_container/3 --> "/container_topology/data/3")

in this PR: single topology for any entity: '/container_project/show/21' --> "/container_project_topology/data/21'

zeari · 2017-02-26T12:35:14Z

app/assets/javascripts/controllers/container_topology/container_topology_controller.js

+      // search for pattern ^/<controler>/<id>$ in the pathname - /ems_container/4
+      var arr = pathname.match('/(.+)/([0-9]+)');
+      type = 'container_topology';
+      id = '/' + arr[2]
    }



@himdel @simon3z I tried to make it a little more clear

simon3z · 2017-02-27T10:31:08Z

@zeari IIUC the special logic you have in container_topology_controller.js is because the url paths for container providers and projects are different:

/ems_container/{id}
/container_project/show/{id}

Is this difference in paths really necessary to keep? So much that it's worth to add special handling logic in the topology?
Would it be hard instead to make the url paths consistent so that we won't need to understand both?

zeari · 2017-02-27T11:26:10Z

@simon3z though the project is going in that direction, I think converting the containers area to 'restful urls' seem is a little big just to satisfy this bit of code 😅

cc @martinpovolny

martinpovolny · 2017-02-27T11:48:52Z

@simon3z : we would like to get all the URLs to the "restful" style. But noone is working on that project at this moment imho.

I think that unification is a great think but agree that we cannot do everything in one PR.

simon3z · 2017-02-27T17:29:12Z

I think that unification is a great think but agree that we cannot do everything in one PR.

@martinpovolny if the plan is to move to the "restful" style, maybe now we have the chance to update the container projects and at the same time avoid extra logic in the topology; it seems worth investigating (unless it's a dramatic change).
Of course I don't expect that to be in the same PR.

martinpovolny · 2017-03-02T09:03:30Z

app/controllers/mixins/more_show_actions.rb

+      @showtype = "topology"
+      drop_breadcrumb(:name => @record.name + _(" (Topology)"),
+                      :url  => "/#{controller_name}/show/#{@record.id}?display=topology")
+    end


we have already show_topology in EmsCommon. But never mind this can be unified in a follow up PR. But please, use the show_link to generate the show links so that this works also for restful? == true.

martinpovolny

please, fix the show link

martinpovolny · 2017-03-02T09:07:19Z

Let's fix the show link, merge this and deal with the rest in follow up PRs.

miq-bot · 2017-03-02T10:03:02Z

Checked commit zeari@38c6a00 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
10 files checked, 22 offenses detected

app/helpers/application_helper/toolbar/container_project_view.rb

❗ - Line 20, Col 3 - Style/IndentArray - Indent the right bracket the same as the first position after the preceding left parenthesis.
❗ - Line 3, Col 5 - Style/IndentArray - Use 2 spaces for indentation in an array, relative to the first position after the preceding left parenthesis.

app/services/container_project_topology_service.rb

❗ - Line 12, Col 55 - Style/IndentHash - Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.
❗ - Line 15, Col 55 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 16, Col 53 - Style/IndentHash - Indent the right brace the same as the start of the line where the left brace is.
❗ - Line 17, Col 51 - Style/MultilineHashBraceLayout - Closing hash brace must be on the same line as the last hash element when opening brace is on the same line as the first hash element.
❗ - Line 18, Col 28 - Style/MultilineHashBraceLayout - Closing hash brace must be on the same line as the last hash element when opening brace is on the same line as the first hash element.
❗ - Line 22, Col 59 - Style/AlignHash - Align the elements of a hash literal if they span more than one line.
❗ - Line 32, Col 1 - Style/EmptyLines - Extra blank line detected.

app/services/container_topology_service_mixin.rb

zeari · 2017-03-02T10:08:10Z

please, fix the show link

Fixed

abonas · 2017-05-14T13:39:23Z

@miq-bot add_label topology

miq-bot added compute/containers wip labels Jan 10, 2017

martinpovolny self-requested a review January 27, 2017 09:51

martinpovolny self-assigned this Jan 27, 2017

zeari commented Feb 2, 2017

View reviewed changes

serenamarie125 approved these changes Feb 2, 2017

View reviewed changes

miq-bot added the unmergeable label Feb 10, 2017

zeari force-pushed the project_level_topology branch from 7015b1c to 9c7c96d Compare February 22, 2017 13:26

miq-bot removed the unmergeable label Feb 22, 2017

zeari force-pushed the project_level_topology branch from 9c7c96d to beba70d Compare February 22, 2017 13:51

zeari changed the title ~~[WIP] Topology for Container Projects~~ Topology for Container Projects Feb 22, 2017

miq-bot added ui and removed wip labels Feb 22, 2017

zeari force-pushed the project_level_topology branch from beba70d to c45c1fa Compare February 22, 2017 20:53

zeari commented Feb 23, 2017

View reviewed changes

zeari force-pushed the project_level_topology branch 3 times, most recently from 27279da to 2fd75cf Compare February 23, 2017 16:07

simon3z reviewed Feb 24, 2017

View reviewed changes

zeari force-pushed the project_level_topology branch from 2fd75cf to db55c81 Compare February 26, 2017 12:17

zeari commented Feb 26, 2017

View reviewed changes

zeari force-pushed the project_level_topology branch from db55c81 to 8f4ad1a Compare February 26, 2017 15:19

martinpovolny reviewed Mar 2, 2017

View reviewed changes

martinpovolny suggested changes Mar 2, 2017

View reviewed changes

topology for container projects

38c6a00

zeari force-pushed the project_level_topology branch from 8f4ad1a to 38c6a00 Compare March 2, 2017 10:00

martinpovolny added enhancement euwe/no labels Mar 2, 2017

martinpovolny merged commit 93befd5 into ManageIQ:master Mar 2, 2017

martinpovolny added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 2, 2017

himdel mentioned this pull request Apr 7, 2017

[WIP] Use a single view for rendering all the topology screens #942

Closed

miq-bot added the topology label May 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Topology for Container Projects #120

Topology for Container Projects #120

zeari commented Jan 10, 2017 •

edited

Loading

miq-bot commented Jan 10, 2017

martinpovolny commented Jan 27, 2017

zeari Feb 2, 2017

zeari Feb 2, 2017

martinpovolny Mar 2, 2017

zeari commented Feb 2, 2017

serenamarie125 commented Feb 2, 2017

zeari commented Feb 2, 2017

serenamarie125 left a comment

miq-bot commented Feb 10, 2017

zeari commented Feb 22, 2017

miq-bot commented Feb 22, 2017

zeari commented Feb 22, 2017

zeari commented Feb 22, 2017

simon3z commented Feb 23, 2017

zeari Feb 23, 2017 •

edited

Loading

simon3z Feb 23, 2017

zeari Feb 23, 2017

simon3z Feb 24, 2017

himdel Feb 24, 2017

zeari Feb 26, 2017 •

edited

Loading

zeari Feb 26, 2017

simon3z commented Feb 27, 2017

zeari commented Feb 27, 2017

martinpovolny commented Feb 27, 2017

simon3z commented Feb 27, 2017

martinpovolny Mar 2, 2017

martinpovolny left a comment

martinpovolny commented Mar 2, 2017

miq-bot commented Mar 2, 2017

zeari commented Mar 2, 2017

abonas commented May 14, 2017

Topology for Container Projects #120

Topology for Container Projects #120

Conversation

zeari commented Jan 10, 2017 • edited Loading

miq-bot commented Jan 10, 2017

martinpovolny commented Jan 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeari commented Feb 2, 2017

serenamarie125 commented Feb 2, 2017

zeari commented Feb 2, 2017

serenamarie125 left a comment

Choose a reason for hiding this comment

miq-bot commented Feb 10, 2017

zeari commented Feb 22, 2017

miq-bot commented Feb 22, 2017

zeari commented Feb 22, 2017

zeari commented Feb 22, 2017

simon3z commented Feb 23, 2017

zeari Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeari Feb 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simon3z commented Feb 27, 2017

zeari commented Feb 27, 2017

martinpovolny commented Feb 27, 2017

simon3z commented Feb 27, 2017

Choose a reason for hiding this comment

martinpovolny left a comment

Choose a reason for hiding this comment

martinpovolny commented Mar 2, 2017

miq-bot commented Mar 2, 2017

zeari commented Mar 2, 2017

abonas commented May 14, 2017

zeari commented Jan 10, 2017 •

edited

Loading

zeari Feb 23, 2017 •

edited

Loading

zeari Feb 26, 2017 •

edited

Loading