add more detail to specific service types #839

juanvallejo · 2016-11-07T17:04:37Z

Related Trello Card:
https://trello.com/c/5dZkZQAX/807-2-details-on-other-service-types-kubernetes-networking

This patch adds more detail, such as ingress points and external IPs, to service types
that support these extra fields (LoadBalancer, etc).

@spadgett @jwforres

juanvallejo · 2016-11-07T17:06:07Z

app/views/browse/service.html

                          >{{ingress.ip}}<span ng-if="!$last">, </span></span>
                      </dd>
+                      <dt ng-if="service.spec.externalIPs.length">External IPs</dt>


Would we want to add endpoints as well for each service?

does the CLI show them?

Yeah, this is the output for describing a service

$ oc describe svc/loadbalancer Name: loadbalancer Namespace: default Labels: app=loadbalancer Selector: app=loadbalancer Type: LoadBalancer IP: 172.30.162.207 External IPs: 172.46.160.65 LoadBalancer Ingress: 172.46.160.65 Port: 8080 8080/TCP NodePort: 8080 30347/TCP Endpoints: <none> Session Affinity: None No events.

jwforres

you need to run hack/install-deps.sh to get updated dependencies, you have a big diff in your dist files because your deps are out of date

jwforres · 2016-11-07T17:42:09Z

app/views/browse/service.html

-                      <dt ng-if="resource.status.loadBalancer.ingress.length">Ingress points</dt>
-                      <dd ng-if="resource.status.loadBalancer.ingress.length">
-                        <span ng-repeat="ingress in resource.status.loadBalancer.ingress"
+                      <dt ng-if="service.status.loadBalancer.ingress.length">Ingress points</dt>


add the : character after the labels for consistency

@juanvallejo Do you mind also changing the labels to headline case like "Ingress Points:" while you're in the file? It looks like I missed a few in #805

@spadgett

Do you mind also changing the labels to headline case like "Ingress Points:" while you're in the file?

Sure, will do

jwforres · 2016-11-07T17:46:21Z

app/views/browse/service.html

                          >{{ingress.ip}}<span ng-if="!$last">, </span></span>
                      </dd>
+                      <dt ng-if="service.spec.externalIPs.length">External IPs</dt>


does the CLI show them?

jwforres · 2016-11-07T18:36:59Z

what does the output look like when there are endpoints

On Mon, Nov 7, 2016 at 1:18 PM, Juan Vallejo [email protected]
wrote:

@juanvallejo commented on this pull request.

In app/views/browse/service.html
#839:
 >{{ingress.ip}}, 
 </dd>
 <dt ng-if="service.spec.externalIPs.length">External IPs</dt>
Yeah, this is the output for describing a service

$ oc describe svc/loadbalancer
Name: loadbalancer
Namespace: default
Labels: app=loadbalancer
Selector: app=loadbalancer
Type: LoadBalancer
IP: 172.30.162.207
External IPs: 172.46.160.65
LoadBalancer Ingress: 172.46.160.65
Port: 8080 8080/TCP
NodePort: 8080 30347/TCP
Endpoints:
Session Affinity: None
No events.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#839, or mute the
thread
https://github.com/notifications/unsubscribe-auth/ABZk7Z2OPbBNSjP2JUxao1Tr_7LIWgzIks5q72uFgaJpZM4KrdXt
.

juanvallejo · 2016-11-07T18:44:02Z

@jwforres

what does the output look like when there are endpoints

$ oc describe svc/idling-echo
Name:                   idling-echo
Namespace:              default
Labels:                 app=idling-echo
Selector:               app=idling-echo
Type:                   ClusterIP
IP:                     172.30.205.85
Port:                   tcp-echo        8675/TCP
Endpoints:              172.17.0.8:8675,172.17.0.9:8675
Port:                   udp-echo        3090/UDP
Endpoints:              172.17.0.8:3090,172.17.0.9:3090
Session Affinity:       None
No events.

jwforres · 2016-11-07T18:58:27Z

which are just the IPs of the pods the service is currently sending traffic
to, correct?

I feel like a better approach in the console is to show the list of pods
the service should be sending traffic to and then part of that should be
confirmation of which ones are currently receiving traffic (which ones
actually have endpoints created)

On Mon, Nov 7, 2016 at 1:44 PM, Juan Vallejo [email protected]
wrote:

@jwforres https://github.com/jwforres

what does the output look like when there are endpoints

$ oc describe svc/idling-echo
Name: idling-echo
Namespace: default
Labels: app=idling-echo
Selector: app=idling-echo
Type: ClusterIP
IP: 172.30.205.85
Port: tcp-echo 8675/TCP
Endpoints: 172.17.0.8:8675,172.17.0.9:8675
Port: udp-echo 3090/UDP
Endpoints: 172.17.0.8:3090,172.17.0.9:3090
Session Affinity: None
No events.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#839 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABZk7TzVL-v_6mdXoOExMU7jywAiRv8Wks5q73F1gaJpZM4KrdXt
.

juanvallejo · 2016-11-08T19:37:48Z

@jwforres @spadgett 7e81227 creates a routes table directive and adds it, along with the existing pods table directive, to the service page.

When a service has no routes to display, it reverts to the old behavior of showing create route:

<span ng-if="!routesForService.length">
  <a ng-href="project/{{project.metadata.name}}/create-route?service={{service.metadata.name}}"  ng-if="'routes' | canI : 'create'">Create route</a>
</span>

A service page with routes and pods would look like:

And a service page with pods that have no endpoints looks like this:

jwforres · 2016-11-09T14:23:23Z

It feels like the tables need something to separate them, headers or something. Right now it just feels like a wall of tables :)

jwforres · 2016-11-09T14:24:19Z

I also don't think I would restrict the width on the Route and Pod tables, the only reason we did it on the ports table before was because it looked odd when it stretched the full width

jwforres · 2016-11-09T15:19:54Z

app/scripts/controllers/service.js

+
+        watches.push(DataService.watch("pods", context, function(pods) {
+          var service = $scope.service;
+          if (!service) {


dont request the service here. Instead call getPodsForService either way, and that function should just return if the service hasn't been loaded yet. The serviceResolved method should also call getPodsForService, this way if the service loads after the pods the association will still happen.

jwforres · 2016-11-09T15:23:24Z

app/scripts/controllers/service.js

+        function getPodsForService(service, pods) {
+          $scope.podsForService = [];
+          var ls = new LabelSelector(service.spec.selector);
+          angular.forEach(pods.by("metadata.name"), function(pod) {


Instead of looping over the pods yourself, just do

$scope.podsForService = ls.select(pods);

jwforres · 2016-11-09T15:42:57Z

app/scripts/controllers/service.js

+
+        watches.push(DataService.watch("endpoints", context, function(endpoints) {
+          $scope.podsWithEndpoints = {};
+          angular.forEach(endpoints.by("metadata.name"), function(endpoint) {


endpoints.by("metadata.name") returns a map of objects indexed by the metadata name, so iterating over that map is wasting cycles, you can do a direct lookup in that map using $routeParams.service.

To follow our code style use _.each when looping over either arrays or hashes.

You don't need to do undefined checks when trying to find JSON subpaths if you use _.get, see the usage below to get targetRef.Kind

var svcEndpoint = endpoints.by("metadata.name")[$routeParams.service]; if (svcEndpoint) { _.each(svcEndpoint.subsets, function(subset) { _.each(subset.addresses, function(address) { if (_.get(address, "targetRef.Kind") === "Pod") { $scope.podsWithEndpoints[address.targetRef.name] = true; } }); }); }

jwforres · 2016-11-09T15:45:10Z

app/views/browse/service.html

                          >{{ingress.ip}}<span ng-if="!$last">, </span></span>
                      </dd>
-                      <dt>Routes:</dt>
-                      <dd>
+                      <dt ng-if="service.spec.externalIPs.length">External IPs:</dt>


You should use ng-if-start and ng-if-end when you have consecutive sibling nodes checking the same thing. We have some old code that didn't do this but any new code should. Fix up the other instances in here like ingress above and routes below

ng-if-start="service.spec.externalIPs.length"

jwforres · 2016-11-09T15:45:18Z

app/views/browse/service.html

-                      <dt>Routes:</dt>
-                      <dd>
+                      <dt ng-if="service.spec.externalIPs.length">External IPs:</dt>
+                      <dd ng-if="service.spec.externalIPs.length">


jwforres · 2016-11-09T15:49:08Z

app/views/browse/service.html

+                      <routes-table routes="routesForService" services="services" custom-name-header="routesForService[0].kind"></routes-table>
+                    </div>
+                    <div ng-if="podsForService.length" class="table-responsive small service-table">
+                      <pods-table pods="podsForService" active-pods="podsWithEndpoints" custom-name-header="podsForService[0].kind"></pods-table>


switching to LabelSelector.select is going to return a map, I would just explicitly set custom-name-header to "Pod" and don't worry about trying to use the kind here

@jwforres Hm, for some reason even when dealing with podsForService as a map, it still allows me to select an item by numerical index, so podsForService[0].kind still returns Pod in this case.

jwforres · 2016-11-09T15:49:37Z

app/views/directives/pods-table.html

@@ -15,6 +15,10 @@
    <tr>
      <td data-title="Name">


also need customNameHeader here, this is what shows up at mobile screen widths

jwforres · 2016-11-09T15:51:37Z

app/views/directives/routes-table.html

+</tbody>
+<tbody ng-repeat="route in routes | orderObjectsByDate : true">
+  <tr>
+    <td data-title="Name">


customNameHeader

juanvallejo · 2016-11-09T19:15:09Z

@jwforres Thanks for the feedback! Review comments addressed:

juanvallejo · 2016-11-09T22:31:40Z

@jwforres Here is the pods' table updated to display a Receiving Traffic column instead of the warning tooltip

juanvallejo · 2016-11-11T16:34:27Z

@jwforres @spadgett Here is what the page would look like with all three tables of the same width, and a black-and-white check when a pod is receiving traffic (this is not a commit, just dev-tools):

EDIT: updated image to replace "yes" / "no" with checks and x's in the pods' table "receiving traffic" column
EDIT2: switched x's and checks in the pods table to use text-success / text-danger

spadgett · 2016-11-11T18:03:57Z

I prefer the 100% width of the ports table, but we might need to center the td content so the arrows are centered. @jwforres ?

Suggest just a check or x for receiving traffic without the "yes/no" text.

jwforres · 2016-11-14T14:13:00Z

@juanvallejo use the text-success class (green) on the check and the text-danger class (red) on the x

jwforres · 2016-11-14T14:14:20Z

If I see I have pods that aren't receiving traffic I probably want to know why. Is it worth having a help tip next to Receiving Traffic that suggests reasons your pod may not be getting any traffic yet?

@spadgett ?

jwforres · 2016-11-14T14:19:41Z

I dunno I actually think i like the ports table shorter, it just doesn't have the content to be full width to me.

But I'm starting to wonder if maybe Routes and Pods should be in tabs instead

Just throwing ideas out, but what if we tied in the words Incoming and Outgoing somehow? I feel like there isn't enough context here to someone that doesn't understand how the networking works. How do they know that the Routes are saying what hostnames are pushing traffic to this service and how do they know that services send traffic to pods. I feel like this is an opportunity for us to make this page better.

cc @ajacobs21e

juanvallejo · 2016-11-16T16:12:30Z

@jwforres

Just throwing ideas out, but what if we tied in the words Incoming and Outgoing somehow? I feel like there isn't enough context here to someone that doesn't understand how the networking works. How do they know that the Routes are saying what hostnames are pushing traffic to this service and how do they know that services send traffic to pods.

Maybe we could have cards similar to the overview page to help display this?
An overall card represents a service, with each of its routes displayed as sets (similar to how container information is displayed on the overview) that contain their hostname / other info with arrows pointing towards the pods donut (also similar to how they are displayed on the overview page, but without the "scale up / scale down" arrows), thoughts?

juanvallejo · 2016-11-16T16:52:12Z

@jwforres @spadgett Page with tabs for each table

spadgett · 2016-11-16T17:10:41Z

I like the tabs now that I've seen it.

Should it be "Routes | Ports | Pods" since that is how the traffic flows?

Are tabs missing from the last screenshot because there are none to show? I'd probably keep the tab and have a message like no routes. Maybe we should put a number with the count in the tab title like we do on the membership page.

spadgett · 2016-11-16T17:16:48Z

If I see I have pods that aren't receiving traffic I probably want to know why. Is it worth having a help tip next to Receiving Traffic that suggests reasons your pod may not be getting any traffic yet?

It would be especially helpful if we can say exactly why, for instance when status is ContainerCreating or the container hasn't passed the readiness check. Then have a generic fallback explaining common reasons if we don't know?

juanvallejo · 2016-11-16T19:21:07Z

@spadgett Thanks for the feedback. Here is the same page with each tab displaying the number of resources in each. If a tab has none to show, it displays a warning.

Empty table

Custom messages if pod not receiving traffic

Default traffic warning

ajacobs21e · 2016-11-16T19:28:57Z

I dunno I actually think i like the ports table shorter, it just doesn't have the content to be full width to me.

Agreed

But I'm starting to wonder if maybe Routes and Pods should be in tabs instead

Tabs within the detail page or tabs between details and events? I am not sure if I like the tabs within tabs. How many rows would we expect each table to have?

Just throwing ideas out, but what if we tied in the words Incoming and Outgoing somehow? I feel like there isn't enough context here to someone that doesn't understand how the networking works. How do they know that the Routes are saying what hostnames are pushing traffic to this service and how do they know that services send traffic to pods. I feel like this is an opportunity for us to make this page better.

As someone that doesn't fully understand networking, this could be helpful. Which word goes where? :)

@jwforres @juanvallejo

ajacobs21e · 2016-11-17T16:07:22Z

How are pods, services, routes, and ports related? Maybe understanding those connections will help me suggest a design.

Would a user want to / need to see more than one table at a time to compare data and/or follow the path?

ajacobs21e · 2016-11-29T20:50:39Z

With only two tables it's ok to leave both tables on the details page with headers "Traffic" and "Pods"

juanvallejo · 2016-11-30T18:55:36Z

@spadgett @ajacobs21e @jwforres

Thanks again for the feedback. Commit c7b83a2 makes a new traffic-table directive that combines routes and ports for a service. Pods are also shown in the same Details tab, rather than in their own tab.

A Learn more link has been added beside the Traffic header that links to Route Types documentation.

Routes and Node Ports are shown in the same column. If there are no node ports, they are not shown at all.

If a service has no routes, but does have ports, the same traffic-table directive is re-used, with any missing route information set to none.

The routes-table directive once again has the same original behavior that was introduced in an earlier commit (150bd60) in this PR:

jwforres · 2016-11-30T19:37:33Z

I think part of what we were saying is that you should not even see "Node Port" in the header if there are no node ports.

…

On Wed, Nov 30, 2016 at 1:55 PM, Juan Vallejo ***@***.***> wrote: @spadgett <https://github.com/spadgett> @ajacobs21e <https://github.com/ajacobs21e> @jwforres <https://github.com/jwforres> Thanks again for the feedback. Commit c3f8c98 <c3f8c98> makes a new traffic-table directive that combines routes and ports for a service. Pods are also shown in the same Details tab, rather than in their own tab. [image: services-page-dns-to-hostname-learn-more-link-traffic-table] <https://cloud.githubusercontent.com/assets/3902875/20765842/12802196-b702-11e6-8e5e-1bc283c2ea40.png> A Learn more link has been added beside the Traffic header that links to Route Types documentation <https://docs.openshift.org/latest/architecture/core_concepts/routes.html#route-types> . [image: service-page-learn-more-link] <https://cloud.githubusercontent.com/assets/3902875/20766034/c10d0116-b702-11e6-8d0c-27bfe64d5b55.png> Routes and Node Ports are shown in the same column. If there are no node ports, they are not shown at all. [image: service-page-with-route-and-nodeport] <https://cloud.githubusercontent.com/assets/3902875/20766421/745171fc-b704-11e6-87d1-6e7753571574.png> If a service has no routes, but does have ports, the same traffic-table directive is re-used, with any missing route information set to none. [image: service-page-with-ports-and-no-routes] <https://cloud.githubusercontent.com/assets/3902875/20766164/503ef344-b703-11e6-86b4-35637cfa3d6e.png> [image: service-page-no-routes-or-ports] <https://cloud.githubusercontent.com/assets/3902875/20766109/16aea9c6-b703-11e6-9b35-7f1ebeb2ad18.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#839 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZk7f924BlDt6ehQn4zH9jz6JtZJje4ks5rDcargaJpZM4KrdXt> .

juanvallejo · 2016-11-30T20:08:11Z

@jwforres @spadgett

I think part of what we were saying is that you should not even see "Node
Port" in the header if there are no node ports.

Thanks, updated the last commit to hide Node Port from the header if there are none to show.

juanvallejo · 2016-12-01T20:01:25Z

app/views/browse/service.html

@@ -62,50 +62,48 @@
                      <dd>{{service.spec.type}}</dd>
                      <dt>IP:</dt>
                      <dd>{{service.spec.clusterIP}}</dd>
+                      <dt ng-if-start="service.spec.externalName">External Name</dt>
+                      <dd ng-if-end>{{service.spec.externalName}}</dd>


@jwforres Added this field to display a service's external name (if any) for the new types of services. https://tcms-openshift.rhcloud.com/case/6047/

PTAL

is external name like a hostname? should this be "External Hostname:"

Either way don't forget the : on the end of the label

@jwforres Thanks, updated it to "External Hostname:", image below:

juanvallejo · 2016-12-06T19:03:23Z

@jwforres thanks for the feedback. The traffic and pods tables no longer have the small class name

The Receiving traffic icon is now an x when a pod is not receiving traffic

and there is now a footnote that links to the routes docs, and services docs.

jwforres · 2016-12-06T19:07:10Z

the footnote should be a lot closer to the bottom of the table. for the icons are you using fa-fw class to make sure they are a fixed width? the x and check looks like different sizes.

…

On Tue, Dec 6, 2016 at 2:03 PM, Juan Vallejo ***@***.***> wrote: @jwforres <https://github.com/jwforres> thanks for the feedback. The traffic and pods tables no longer have the small class name [image: services-page-bigger-tables-receiving-traffic-tweaks] <https://cloud.githubusercontent.com/assets/3902875/20938729/296c30d6-bbba-11e6-8ddb-c23c6c60cc9e.png> The Receiving traffic icon is now an x when a pod is not receiving traffic [image: services-page-receiving-traffic-max] <https://cloud.githubusercontent.com/assets/3902875/20938841/8f02f93e-bbba-11e6-9863-6334de517020.png> and there is now a footnote that links to the routes <https://docs.openshift.org/latest/architecture/core_concepts/routes.html#route-types> docs, and services <https://docs.openshift.org/latest/architecture/core_concepts/pods_and_services.html#services> docs. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#839 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZk7Tc30xyNeD0hZKLxA3979sCpQD2Zks5rFbF8gaJpZM4KrdXt> .

juanvallejo · 2016-12-06T20:55:13Z

@jwforres added support for a "footnote" to the traffic table that decreases its bottom margin to bring it closer:

Added the fa-fw class to both the check and the x icons in the pod's table, but I still think they seem to have different sizes:

jwforres · 2016-12-07T16:06:09Z

app/views/directives/pods-table.html

@@ -39,11 +39,11 @@
      <td data-title="Age"><span am-time-ago="pod.metadata.creationTimestamp" am-without-suffix="true"></span></td>
      <td ng-if="activePods" data-title="Receiving Traffic">
        <span ng-if="activePods[pod.metadata.name]">
-          <status-icon status="'Succeeded'" disable-animation></status-icon>
+          <status-icon fixed-width="true" status="'Succeeded'" disable-animation></status-icon>
        </span>
        <span ng-if="!activePods[pod.metadata.name]">
           <span data-toggle="popover" data-trigger="hover" data-content="{{podFailureReasons[pod.status.phase] || 'This pod has no endpoints and is not accepting traffic.'}}" style="cursor: help; padding-left: 5px;">


i think your problem is the padding-left that is on this span, thats what is offsetting this icon compared to the other

i dont think the padding is needed, try just removing it

jwforres

not done reviewing yet, but first chunk of feedback

jwforres · 2016-12-07T16:11:05Z

app/scripts/controllers/service.js

+      $scope.portsForRoutes = {};
+      var orderedPorts = $filter("orderBy")(service.spec.ports, "port");
+
+      for(var i = 0; i < routes.length; i++) {


we don't use for loops, we use _.each for all iteration

_.each(routes, function(route) { });

jwforres · 2016-12-07T16:13:29Z

app/scripts/controllers/service.js

+      var orderedPorts = $filter("orderBy")(service.spec.ports, "port");
+
+      for(var i = 0; i < routes.length; i++) {
+        $scope.portsForRoutes[routes[i].metadata.name] = orderedPorts[i];


This block doesn't seem right to me. A route can specify what port it uses. You might have multiple routes targeting the same ports on the service. You might have some service ports that have no route associated with it.

jwforres · 2016-12-07T16:15:43Z

app/scripts/controllers/service.js

    var serviceResolved = function(service, action) {
      $scope.loaded = true;
      $scope.service = service;
+
+      getPodsForService(service, $scope.pods);


service and pods are already on the scope, you don't need to pass them into the functions

jwforres · 2016-12-07T16:19:08Z

app/scripts/controllers/service.js

+        }));
+
+        watches.push(DataService.watch("pods", context, function(pods) {
+          $scope.pods = [];


there shouldn't be any reason for converting the pod map to an array, you also probably have no reason to have all pods on the scope, you only care about the pods for the service from within the view. You can use a local variable to the controller to keep track of the full set of pods.

jwforres · 2016-12-07T16:23:13Z

app/scripts/controllers/service.js

+
+        watches.push(DataService.watch("endpoints", context, function(endpoints) {
+          $scope.podsWithEndpoints = {};
+          var svcEndpoint  = endpoints.by("metadata.name");


i would probably just make this a one-liner instead of initializing a variable as a map and then turning it into a singleton

var svcEndpoint = endpoints.by("metadata.name")[$routeParams.service];

jwforres · 2016-12-07T16:28:39Z

app/views/browse/service.html

@@ -62,50 +62,53 @@
                      <dd>{{service.spec.type}}</dd>
                      <dt>IP:</dt>
                      <dd>{{service.spec.clusterIP}}</dd>
+                      <dt>Hostname:</dt>
+                      <dd>
+                        {{service.metadata.name + '.' + service.metadata.namespace + '.svc'}}


Don't concatenate when it isn't necessary

{{service.metadata.name}}.{{service.metadata.namespace}}.svc

jwforres · 2016-12-07T16:30:32Z

app/views/browse/service.html

+                        <span ng-repeat="externalIP in service.spec.externalIPs"
+                          >{{externalIP}}<span ng-if="!$last">, </span></span>
+                      </dd>
+                      <dt ng-if="!routesForService.length">Routes:</dt>


switch to ng-if-start

jwforres · 2016-12-07T16:30:39Z

app/views/browse/service.html

+                          >{{externalIP}}<span ng-if="!$last">, </span></span>
+                      </dd>
+                      <dt ng-if="!routesForService.length">Routes:</dt>
+                      <dd ng-if="!routesForService.length">


jwforres · 2016-12-07T16:31:00Z

app/views/browse/service.html

+                          >{{externalIP}}<span ng-if="!$last">, </span></span>
+                      </dd>
+                      <dt ng-if="!routesForService.length">Routes:</dt>
+                      <dd ng-if="!routesForService.length">
                        <span ng-if="!routesForService.length">


this if check is unnecessary, the one above it covers this

jwforres · 2016-12-07T16:31:27Z

app/views/browse/service.html

+                          >{{externalIP}}<span ng-if="!$last">, </span></span>
+                      </dd>
+                      <dt ng-if="!routesForService.length">Routes:</dt>
+                      <dd ng-if="!routesForService.length">
                        <span ng-if="!routesForService.length">
                          <a ng-href="project/{{project.metadata.name}}/create-route?service={{service.metadata.name}}"  ng-if="'routes' | canI : 'create'">Create route</a>


if user doesn't have access to create routes, this should say none

jwforres · 2016-12-07T17:54:14Z

app/views/browse/service.html

+                      <traffic-table routes="routesForService" ports="portsForRoutes" services="services" show-node-ports="showNodePorts" custom-name-header="routesForService[0].kind" has-footer="true"></traffic-table>
+                    </div>
+                    Learn more about
+                    <span class="learn-more-inline" style="margin-left:0;">


is learn-more-inline actually giving you anything here when you have these links embedded in a sentence? i also don't actually like the external link icon on these links, it looks strange in this context

jwforres · 2016-12-07T17:55:48Z

app/views/browse/service.html

+                      <a ng-href="{{'services' | helpLink}}" target="_blank">
+                      services <i class="fa fa-external-link"></i>
+                      </a>
+                    </span>.


because you have newlines in the template the . is going to have whitespace between it and the previous link

jwforres · 2016-12-07T17:57:12Z

app/views/directives/pods-table.html

        <a href="{{pod | navigateResourceURL}}">{{pod.metadata.name}}</a>
+        <span ng-if="pod | isTroubledPod">


didnt we take this out of the pods table in your other PR

jwforres · 2016-12-07T17:58:23Z

app/views/directives/pods-table.html

+        <span ng-if="!activePods[pod.metadata.name]">
+           <span data-toggle="popover" data-trigger="hover" data-content="{{podFailureReasons[pod.status.phase] || 'This pod has no endpoints and is not accepting traffic.'}}" style="cursor: help; padding-left: 5px;">
+            <span class="fa fa-fw fa-times text-danger" aria-hidden="true" data-toggle="tooltip" style="cursor: help;"></span>
+            <span class="sr-only">{{pod.status.phase}}</span>


the phase is not the right sr-only content here, this should probably be either "No" or "Not receiving traffic"

jwforres · 2016-12-07T17:59:12Z

app/views/directives/pods-table.html

@@ -33,6 +37,17 @@
      <td data-title="Ready">{{pod | numContainersReady}}/{{pod.spec.containers.length}}</td>
      <td data-title="Restarts">{{pod | numContainerRestarts}}</td>
      <td data-title="Age"><span am-time-ago="pod.metadata.creationTimestamp" am-without-suffix="true"></span></td>
+      <td ng-if="activePods" data-title="Receiving Traffic">
+        <span ng-if="activePods[pod.metadata.name]">
+          <status-icon fixed-width="true" status="'Succeeded'" disable-animation></status-icon>


using status-icon here is actually going to cause the sr-only text to say "Succeeded" when really it should be something like "Yes"

jwforres · 2016-12-07T18:01:09Z

app/views/directives/traffic-table.html

@@ -0,0 +1,84 @@
+<!-- Table combining routes and ports for a service -->
+<table class="table table-bordered table-hover table-mobile" style="{{ hasFooter ? 'margin-bottom:5px;' : ''}}">


when i saw hasFooter i expected the footer to actually be part of the directive, not to just change the styles of the table. i would either make the footer part of the directive, or you can use negative margins on the footer.

jwforres · 2016-12-07T18:03:09Z

app/views/directives/traffic-table.html

+<thead>
+  <tr>
+    <th ng-if="!showNodePorts">{{customNameHeader || 'Route'}}</th>
+    <th ng-if="showNodePorts">{{customNameHeader || 'Route'}} / Node Port</th>


I'm not sure if this will screw up a screen reader making these different <th> elements, when the content can change dynamically, instead I would probably do

<th>{{customNameHeader || 'Route'}} / Node Port</th>

jwforres · 2016-12-07T18:05:05Z

app/views/directives/traffic-table.html

+<tbody ng-if="(routes | hashSize) == 0 && (ports | hashSize) == 0">
+  <tr><td colspan="7"><em>{{emptyMessage || 'No routes or ports to show'}}</em></td></tr>
+</tbody>
+<tbody ng-if="(routes | hashSize) > 0">


the route / port iteration and association logic in here isn't right, lets sync up later

jwforres · 2016-12-07T18:07:24Z

app/views/directives/traffic-table.html

+</tbody>
+<tbody ng-if="(routes | hashSize) > 0">
+  <tr ng-repeat="route in routes | orderObjectsByDate : true">
+    <td data-title="{{ customNameHeader || 'Route' }}">


this data-title needs to match the column header and include / Node Port when the other one does.

juanvallejo · 2016-12-08T20:09:24Z

@jwforres thanks for your help and feedback, review comments addressed.

Traffic table now shows each route's information for every service port that it reaches.

Ports that have no routes linked to them are also displayed

"No traffic" icon in the pod's table is now aligned properly

jwforres · 2016-12-08T21:08:23Z

app/scripts/controllers/service.js

+      _.each($scope.service.spec.ports, function(port) {
+        var reachedByRoute = false;
+        _.each($scope.routesForService, function(route) {
+          if(!route.spec.port || route.spec.port.targetPort === port.name ||


@spadgett you are more familiar with the route / port stuff, can you review this section

It looks correct.

jwforres · 2016-12-08T21:10:06Z

app/scripts/controllers/service.js

+        _.each($scope.routesForService, function(route) {
+          if(!route.spec.port || route.spec.port.targetPort === port.name ||
+            route.spec.port.targetPort === port.targetPort) {
+            if(!$scope.portsByRoute[route.metadata.name]) {


we usually do this if block as a one-liner

$scope.portsByRoute[route.metadata.name] = $scope.portsByRoute[route.metadata.name] || [];

jwforres · 2016-12-08T21:10:47Z

app/scripts/controllers/service.js

+        });
+
+        if(!reachedByRoute) {
+          if(!$scope.portsByRoute['']) {


$scope.portsByRoute[''] = $scope.portsByRoute[''] || [];

jwforres · 2016-12-08T21:12:17Z

app/scripts/controllers/service.js

+
+        watches.push(DataService.watch("endpoints", context, function(endpoints) {
+          $scope.podsWithEndpoints = {};
+          var svcEndpoint  = endpoints.by("metadata.name")[$routeParams.service];


its a nit but you have an extra whitespace after svcEndpoint

jwforres · 2016-12-08T21:14:27Z

app/views/directives/pods-table.html

@@ -33,6 +34,18 @@
      <td data-title="Ready">{{pod | numContainersReady}}/{{pod.spec.containers.length}}</td>
      <td data-title="Restarts">{{pod | numContainerRestarts}}</td>
      <td data-title="Age"><span am-time-ago="pod.metadata.creationTimestamp" am-without-suffix="true"></span></td>
+      <td ng-if="activePods" data-title="Receiving Traffic">
+        <span ng-if="activePods[pod.metadata.name]">
+          <span class="fa fa-check text-success" aria-hidden="true"></span>


missing fa-fw

jwforres · 2016-12-08T21:16:31Z

app/views/directives/traffic-table.html

+    </tr>
+    <tr ng-repeat-end style="display:none;"></tr>
+    <tr ng-repeat="port in portsByRoute['']">
+      <td data-title="{{customNameHeader || 'Route'}}"><span class="text-muted">none</span></td>


you are missing node port for this case

juanvallejo · 2016-12-08T22:24:14Z

@jwforres Thanks, review comments addressed.

NodePort case is now handled when there are 0 or >0 routes

jwforres · 2016-12-09T14:04:51Z

@juanvallejo looks good now, can you squash your commits

Related Trello Card: https://trello.com/c/5dZkZQAX/807-2-details-on-other-service-types-kubernetes-networking This patch adds more detail, such as ingress points, to service types that support these extra fields (LoadBalancer, etc). It also replaces single-line descriptions, for listing routes and pods related to the current service, with route and pod table directives. It adds warning tooltips to the pod table directive to rows displaying pods with no endpoints. This patch further adds an optional field to pod and route directives for setting a custom 'Name' column header value.

juanvallejo · 2016-12-09T15:43:31Z

@jwforres

looks good now, can you squash your commits

Thanks! Done

jwforres · 2016-12-09T15:44:35Z

[merge]

openshift-bot · 2016-12-09T15:47:21Z

Evaluated for origin web console merge up to 6a6bd16

openshift-bot · 2016-12-09T15:47:31Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/832/) (Base Commit: 1fb702b)

juanvallejo commented Nov 7, 2016

View reviewed changes

jwforres requested changes Nov 7, 2016

View reviewed changes

juanvallejo force-pushed the jvallejo/add-detail-other-svc-types branch 2 times, most recently from f30ac7b to 21aabbd Compare November 7, 2016 18:43

juanvallejo force-pushed the jvallejo/add-detail-other-svc-types branch 2 times, most recently from eb360d2 to 7e81227 Compare November 8, 2016 19:32

jwforres reviewed Nov 9, 2016

View reviewed changes

juanvallejo force-pushed the jvallejo/add-detail-other-svc-types branch from a38ca01 to 546a661 Compare November 9, 2016 19:19

juanvallejo force-pushed the jvallejo/add-detail-other-svc-types branch from fe814e1 to fa24643 Compare November 9, 2016 22:38

juanvallejo force-pushed the jvallejo/add-detail-other-svc-types branch from f1333b3 to 0359c30 Compare November 16, 2016 16:58

juanvallejo force-pushed the jvallejo/add-detail-other-svc-types branch from c3f8c98 to c7b83a2 Compare November 30, 2016 20:07

juanvallejo commented Dec 1, 2016

View reviewed changes

juanvallejo force-pushed the jvallejo/add-detail-other-svc-types branch from 0d4e584 to 12f08dd Compare December 1, 2016 21:12

juanvallejo force-pushed the jvallejo/add-detail-other-svc-types branch from cb9421b to a5f6916 Compare December 6, 2016 21:48

jwforres reviewed Dec 7, 2016

View reviewed changes

jwforres requested changes Dec 7, 2016

View reviewed changes

juanvallejo force-pushed the jvallejo/add-detail-other-svc-types branch 2 times, most recently from b5e6aad to fee3da9 Compare December 8, 2016 20:09

jwforres reviewed Dec 8, 2016

View reviewed changes

jwforres requested changes Dec 8, 2016

View reviewed changes

juanvallejo force-pushed the jvallejo/add-detail-other-svc-types branch from c570b50 to 6a6bd16 Compare December 9, 2016 15:39

jwforres approved these changes Dec 9, 2016

View reviewed changes

openshift-bot merged commit 1265ebc into openshift:master Dec 9, 2016

juanvallejo deleted the jvallejo/add-detail-other-svc-types branch December 9, 2016 16:03

		<a href="{{pod \| navigateResourceURL}}">{{pod.metadata.name}}</a>
		<span ng-if="pod \| isTroubledPod">

		@@ -0,0 +1,84 @@
		<!-- Table combining routes and ports for a service -->
		<table class="table table-bordered table-hover table-mobile" style="{{ hasFooter ? 'margin-bottom:5px;' : ''}}">

add more detail to specific service types #839

add more detail to specific service types #839

Conversation

juanvallejo commented Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwforres left a 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

Choose a reason for hiding this comment

jwforres commented Nov 7, 2016

@juanvallejo commented on this pull request.

juanvallejo commented Nov 7, 2016

jwforres commented Nov 7, 2016

juanvallejo commented Nov 8, 2016 • edited Loading

jwforres commented Nov 9, 2016

jwforres commented Nov 9, 2016

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

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

juanvallejo commented Nov 9, 2016

juanvallejo commented Nov 9, 2016 • edited Loading

juanvallejo commented Nov 11, 2016 • edited Loading

spadgett commented Nov 11, 2016

jwforres commented Nov 14, 2016

jwforres commented Nov 14, 2016

jwforres commented Nov 14, 2016

juanvallejo commented Nov 16, 2016

juanvallejo commented Nov 16, 2016

spadgett commented Nov 16, 2016

spadgett commented Nov 16, 2016

juanvallejo commented Nov 16, 2016 • edited Loading

ajacobs21e commented Nov 16, 2016

ajacobs21e commented Nov 17, 2016

ajacobs21e commented Nov 29, 2016

juanvallejo commented Nov 30, 2016 • edited Loading

jwforres commented Nov 30, 2016 via email

juanvallejo commented Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo commented Dec 6, 2016

jwforres commented Dec 6, 2016 via email

juanvallejo commented Dec 6, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jwforres left a 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

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

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

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

Choose a reason for hiding this comment

juanvallejo commented Dec 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

juanvallejo commented Nov 7, 2016 •

edited

Loading

juanvallejo commented Nov 8, 2016 •

edited

Loading

juanvallejo commented Nov 9, 2016 •

edited

Loading

juanvallejo commented Nov 11, 2016 •

edited

Loading

juanvallejo commented Nov 16, 2016 •

edited

Loading

juanvallejo commented Nov 30, 2016 •

edited

Loading

juanvallejo commented Nov 30, 2016 •

edited

Loading

juanvallejo commented Dec 8, 2016 •

edited

Loading

openshift-bot commented Dec 9, 2016 •

edited

Loading