Service Instance Details Pages #1906

cdcabrera · 2017-08-03T14:44:48Z

Prep for update around service instances and bindings. Page for service instances.

Related to Origin Design PR #52
Dependency on Origin-Web-Common PR 164

spadgett · 2017-08-07T20:45:22Z

@cdcabrera Let me know when you'd like a review, thanks!

cdcabrera · 2017-08-08T13:06:24Z

@spadgett ~~going through and generating screenshots along with questions~~

Edit: went ahead and worked a few questions into the review response comments

spadgett

@cdcabrera Thanks. I realize this is a WIP, but I've added some initial comments.

spadgett · 2017-08-08T13:14:07Z

app/scripts/app.js

@@ -271,6 +271,16 @@ angular
        controller: 'ServiceController',
        reloadOnSearch: false
      })
+      .when('/project/:project/browse/provisioned', {


I think it would be good to match the API kind in the code even if we prefer to it as "Provisioned Service" in the UI. So I'd suggest /browse/service-instances/ and /browser/services-instances/:instance with corresponding names to the controller and views.

@spadgett what about matching file names too?

Yes, let's be consistent with the filenames also

spadgett · 2017-08-08T13:15:26Z

app/scripts/controllers/provisionedService.js

+                                             $routeParams,
+                                             DataService,
+                                             ProjectsService,
+                                             $filter) {


Nit: I like to keep the lists alphabetized to keep them more manageable.

spadgett · 2017-08-08T13:15:42Z

app/scripts/controllers/provisionedService.js

+    $scope.serviceClasses = {};
+    $scope.alerts = {};
+    //$scope.renderOptions = $scope.renderOptions || {};
+    //$scope.renderOptions.hideFilterWidget = true;


The two commented out lines can be removed.

spadgett · 2017-08-08T13:16:44Z

app/scripts/controllers/provisionedService.js

+
+    $scope.podFailureReasons = {
+      "Pending": "This pod will not receive traffic until all of its containers have been created."
+    };


Looks like you have a fair amount of code from the service controller that is not needed here.

Indeed, used the service controller as the basis. Anything we can wipe out... keep those comments coming

spadgett · 2017-08-08T13:18:13Z

app/scripts/controllers/provisionedService.js

+    var watches = [];
+
+    // receives routes for the current service and maps service ports to each route name
+    var getPortsByRoute = function() {


This entire function can be removed.

spadgett · 2017-08-08T13:38:21Z

app/views/provisioned.html

+                    <th>Age</th>-->
+                  </tr>
+                </thead>
+                <tbody ng-if="(serviceInstances | hashSize) === 0">


Use | size instead of | hashSize

spadgett · 2017-08-08T13:38:30Z

app/views/provisioned.html

+                    <td colspan="5"><em>{{emptyMessage}}</em></td>
+                  </tr>
+                </tbody>
+                <tbody ng-if="(serviceInstances | hashSize) > 0">


spadgett · 2017-08-08T13:39:04Z

app/views/provisioned.html

+                  </tr>
+                </tbody>
+                <tbody ng-if="(serviceInstances | hashSize) > 0">
+                  <tr ng-repeat="serviceInstance in serviceInstances | orderObjectsByDate : true">


Better to sort in the controller, not the view.

Use track by (serviceInstance | uid)

spadgett · 2017-08-08T13:39:19Z

app/views/provisioned.html

+                </tbody>
+                <tbody ng-if="(serviceInstances | hashSize) > 0">
+                  <tr ng-repeat="serviceInstance in serviceInstances | orderObjectsByDate : true">
+                    <td data-title="Name"><a href="{{serviceInstance | navigateResourceURL}}">{{serviceInstance | serviceInstanceDisplayName:serviceClasses}}</a></td>


nit: ng-href

spadgett · 2017-08-08T13:39:59Z

app/views/provisioned.html

+                    </td>
+                    <td data-title="Status">
+                      <div row class="status">
+                        <status-icon status="serviceInstance | deploymentStatus" disable-animation></status-icon>


deploymentStatus is not correct here. You'll need to look at the overview service instance row to see how we determine status.

jeff-phillips-18 · 2017-08-08T16:49:41Z

app/scripts/controllers/provisionedServices.js

+
+    // TODO: code duplicated from directives/bindService.js
+    // extract & share
+    var sortServiceInstances = function() {


https://github.com/jeff-phillips-18/origin-web-console/blob/e4dbd7f0fa9729e77d7d734650286d75bb800c1c/app/scripts/controllers/overview.js#L1152

jeff-phillips-18 · 2017-08-08T17:00:52Z

app/scripts/controllers/provisionedServices.js

+    };
+
+    var groupBindings = function() {
+      // Build two maps:


See https://github.com/jeff-phillips-18/origin-web-console/blob/e4dbd7f0fa9729e77d7d734650286d75bb800c1c/app/scripts/controllers/overview.js#L1084

spadgett · 2017-08-11T19:22:45Z

app/scripts/controllers/provisionedServices.js

+        return {};
+      }
+      return _.get(state, ['notificationsByObjectUID', uid], {});
+    };*/


This can go.

spadgett · 2017-08-11T19:22:52Z

app/scripts/controllers/provisionedServices.js

+      DataService.list("secrets", context, null, { errorNotification: false }).then(function(secretData) {
+        state.secrets = secretData.by("metadata.name");
+      });
+    }, 300);*/


This can go.

spadgett · 2017-08-11T19:28:15Z

app/scripts/controllers/provisionedService.js

+              group: 'servicecatalog.k8s.io',
+              resource: 'instances'
+            }, $routeParams.instance, context, serviceResolved));
+


watches.push(DataService.watch({ group: 'servicecatalog.k8s.io', resource: 'bindings' }, context, function(bindings) { $scope.bindings = _.filter(bindings.by('metadata.name'), { spec: { instanceRef: { name: instance.metadata.name } } }); }));

spadgett · 2017-08-11T19:28:42Z

app/scripts/controllers/provisionedService.js

+          setDisplayName();
+        });
+
+        watches.push(DataService.watch({


This watch can go since you're already watching the instance for this page above.

cdcabrera · 2017-08-17T15:29:42Z

@spadgett & @jeff-phillips-18 additional updates...

Short checklist of things that may need to be altered

the Plan name/description display on service instance detail and the controller aspect
filters, added a couple of simple filters to help with the status around service instance, not keen on them but they did help simplify the process for display
- service-instance.html
- service-instances.html
additions around resource-service-bindings component line 42 that haven't been moved to the origin-web-common repo
the event tab around the service instance detail view is non-functioning

spadgett · 2017-08-17T18:28:05Z

app/scripts/app.js

@@ -271,6 +271,16 @@ angular
        controller: 'ServiceController',
        reloadOnSearch: false
      })
+      .when('/project/:project/browse/service-instances', {


Note that upstream it will either be called ServiceInstance or ServiceCatalogInstance. What you have now looks good, but we might change the name to align with the final name in the upstream service catalog.

(Nothing to change right now)

Following up to say the name upstream will be ServiceInstance, so what you have is good 👍

spadgett · 2017-08-17T18:30:26Z

app/scripts/constants.js

+                return _.get(window.OPENSHIFT_CONSTANTS, 'ENABLE_TECH_PREVIEW_FEATURE.service_catalog_landing_page');
+              },
+              canI: {
+                resource: 'rolebindings',


resource: { group: 'servicecatalog.k8s.io', resource: 'instances' }

spadgett · 2017-08-17T18:30:53Z

app/scripts/controllers/serviceInstance.js

+                                                        $filter,
+                                                        $routeParams,
+                                                        DataService,
+                                                        ProjectsService) {


indentation

spadgett · 2017-08-17T18:31:53Z

app/scripts/controllers/serviceInstance.js

+
+      var serviceClassName = _.get($scope.serviceInstance.spec, 'serviceClassName');
+      $scope.serviceClass = _.get($scope.serviceClasses, [serviceClassName]);
+      $scope.plan = _.find(_.get($scope.serviceClass, 'plans'), {name: 'default'});


You don't want name: 'default'. You want whatever plan is set in the service instance

$scope.plan = _.find(_.get($scope.serviceClass, 'plans'), {name: $scope.serviceInstance.spec.planName });

spadgett · 2017-08-17T18:32:31Z

app/scripts/controllers/serviceInstance.js

+      $scope.displayName = $filter('serviceInstanceDisplayName')($scope.serviceInstance, $scope.serviceClasses);
+    };*/
+
+    var setDescription = function() {


Maybe updateDescriptionAndPlan since it's not just description, or possibly updateServiceClassMetadata

spadgett · 2017-08-17T19:14:51Z

app/scripts/filters/resources.js

+      return status;
+    };
+  })
+  .filter('serviceInstanceMessage', function(statusConditionFilter) {


Maybe 'readyConditionMessage'

spadgett · 2017-08-17T19:19:54Z

app/views/browse/service-instance.html

+                    <a ng-href="{{serviceInstance | editYamlURL}}" role="button">Edit YAML</a>
+                  </li>
+                  <li ng-if="{resource: 'instances', group: 'servicecatalog.k8s.io'} | canI : 'delete'">
+                    <delete-link


Probably want the same dialog as used here:

https://github.com/openshift/origin-web-console/blob/master/app/scripts/directives/overview/serviceInstanceRow.js#L84

Let's make it a common component that we can reuse. We'll eventually need to handle bindings.

In converting this over to a service, did a comparison on the current directive/component delete link looks like there are obvious "copy" differences, but the core of it is the same. Passing in the additional parameter/option/attribute for "group" and wrapping some of the display logic/ng-ifs with a service instance could solve this while separating out the DOM manipulation/modal call from the service

Purpose we either modify the existing, or create a new component, for the delete link for "service instances". This may make linking to the new "Service Instances Service" unnecessary unless we want to further eliminate the delete logic from the delete link directive/component... a little like pulling on a thread with this one

@spadgett after going over the modal designs, talked with @ncameronbritt about the style. The style for...

fits more inline with the design aspect as opposed to the current...

Since this seems to fit partially with the "Service Instances Service" I can do a few things..

leave the new "Service Instances Service" as is, free floating and convert the existing Provisioned Service "delete" links over to the preferred modal design

leave the new "Service Instances Service" as is and leave the delete links/buttons alone

just convert the new "Service Instances Service" by removing the DOM manipulation/modal

or leave everything as is

@beanh66

We probably want something closer to the first dialog, but I'd rather do that in a separate PR. Let's reuse what we have for now and revisit the design for both overview and this page later.

spadgett · 2017-08-17T19:33:46Z

app/views/browse/service-instance.html

+                          {{plan.description}}
+                        </p>
+                        <dl class="dl-horizontal left">
+                          <dt ng-if-start="serviceClass.description">Description:</dt>


Probably want this above the plan details

We should show externalMetadata.longDescription if it's there even if description is not.

spadgett · 2017-08-17T19:35:29Z

app/views/browse/service-instance.html

+                          <dt ng-if-start="serviceInstance | serviceInstanceMessage">Status Reason:</dt>
+                          <dd ng-if-end>
+                            {{serviceInstance | serviceInstanceMessage}}
+                          </dd>


We've used a somewhat different layout for status on the route page and the build config page

spadgett · 2017-08-17T19:36:04Z

app/views/browse/service-instance.html

+                        </dl>
+                      </div>
+                      <div class="col-lg-6">
+                        <resource-service-bindings


We might rename this component to just service-bindings if we're using it for service instances too.

I can do this, it's going to affect a handful of files.

spadgett · 2017-08-18T16:24:43Z

[test]

serenamarie125 · 2017-08-18T20:15:17Z

@beanh66 Can you take a look at this, based on your design?

spadgett · 2017-08-21T12:37:33Z

@beanh66 @serenamarie125 Just a heads up that the PR is still WIP, so some things are missing.

beanh66 · 2017-08-21T15:00:24Z

Okay no problem @spadgett! @cdcabrera let me know when this is ready for UI review and we can meet if that's easiest.

cdcabrera · 2017-08-24T14:57:45Z

Reference point for PR #1961 Consider looking at breaking out another component for the status from service instance row.

spadgett · 2017-09-18T15:52:11Z

@cdcabrera I'm seeing these errors on the overview in the console. Do you see this as well? Doesn't happen on master.

angular.js:14199 TypeError: resource.indexOf is not a function
    at normalizeResource (origin-web-common.js:2162)
    at Object.toResourceGroupVersion (origin-web-common.js:2150)
    at Object.canI (origin-web-common.js:2901)
    at ChildScope.$scope.show (nav.js:85)
    at fn (eval at compile (angular.js:15126), <anonymous>:4:297)
    at Scope.$digest (angular.js:17828)
    at angular.js:18033
    at completeOutstandingRequest (angular.js:6045)
    at angular.js:6324

spadgett · 2017-09-18T15:58:55Z

app/scripts/constants.js

+                  resource: 'serviceinstances'
+                },
+                verb: 'list'
+              }


This should be

canI: { resource: 'serviceinstances', group: 'servicecatalog.k8s.io', verb: 'list' }

that would break, sure thing

spadgett · 2017-09-18T16:08:49Z

app/scripts/constants.js

+              ],
+              isValid: function(){
+                return _.get(window.OPENSHIFT_CONSTANTS, 'ENABLE_TECH_PREVIEW_FEATURE.service_catalog_landing_page');
+              },


Should be able to remove isValid now that #2078 is in the merge queue. The canI check below should hide the menu item when service catalog is not enabled.

#2078 has merged

cdcabrera · 2017-09-18T16:21:16Z

@spadgett, yeah looks like I need to verify something and possible rebase

spadgett · 2017-09-18T16:23:57Z

Yeah you'll want to rebase to pick up #2048 to properly test

cdcabrera · 2017-09-18T17:25:41Z

@spadgett rebased, don't see any errors on my end.

Do we still need the line in constants.js around isValid for the navigation?

isValid: function(){
  return _.get(window.OPENSHIFT_CONSTANTS, 'ENABLE_TECH_PREVIEW_FEATURE.service_catalog_landing_page');
},

spadgett · 2017-09-18T17:27:00Z

You can remove isValid now

spadgett · 2017-09-18T17:27:57Z

don't see any errors on my end.

See my comment above on the canI check for the nav item. That's the cause.

#1906 (comment)

spadgett · 2017-09-18T17:33:08Z

app/views/browse/service-instance.html

+          <div ng-if="!loaded" class="mar-top-xl">Loading...</div>
+          <div ng-if="serviceInstance">
+            <h1 class="contains-actions">
+              <div class="pull-right dropdown" ng-hide="!('instances' | canIDoAny)">


Needs to be serviceInstances here or the menu will never show up.

<div class="pull-right dropdown" ng-if="'serviceInstances' | canIDoAny">

spadgett · 2017-09-18T17:55:08Z

You need to update resourceServiceBindings.js with

  ctrl.showBindings = CatalogService.SERVICE_CATALOG_ENABLED &&
                      (_.get(ctrl, 'apiObject.kind') === 'ServiceInstance' || enableTechPreviewFeature('pod_presets'));

https://github.com/openshift/origin-web-console/blob/master/app/scripts/directives/resourceServiceBindings.js#L30

spadgett

Thanks @cdcabrera

spadgett · 2017-09-18T18:51:41Z

[merge]

spadgett · 2017-09-18T20:00:02Z

[merge]

spadgett · 2017-09-18T20:29:49Z

Origin Web Console Test Results: FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_web_console/232/) (Base Commit: 181d896) (PR Branch Commit: 0fd1a62)

Flake #1685

[test]

spadgett · 2017-09-18T21:59:59Z

@cdcabrera You have a dist mismatch. Try rebase, bower install, and grunt build

cdcabrera · 2017-09-18T22:21:23Z

Can do @spadgett

Updates for service instances. Prep to allow updates to chosen parameters for instance and bindings.

openshift-bot · 2017-09-18T22:39:19Z

Evaluated for origin web console test up to f015611

spadgett · 2017-09-18T22:41:31Z

[merge]

openshift-bot · 2017-09-18T22:47:18Z

Evaluated for origin web console merge up to f015611

openshift-bot · 2017-09-18T22:47:28Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/219/) (Base Commit: d06b7c0) (PR Branch Commit: f015611)

openshift-bot · 2017-09-18T23:02:58Z

Origin Web Console Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_web_console/234/) (Base Commit: d06b7c0) (PR Branch Commit: f015611)

cdcabrera force-pushed the service-instances-bindings branch from 8eed8fe to d5ce3be Compare August 7, 2017 14:21

cdcabrera requested a review from spadgett August 8, 2017 13:03

spadgett reviewed Aug 8, 2017

View reviewed changes

cdcabrera mentioned this pull request Aug 8, 2017

New Provisioned Service Details page and Edit Action for Plan/Configuration/Bindings openshift/openshift-origin-design#52

Merged

jeff-phillips-18 reviewed Aug 8, 2017

View reviewed changes

cdcabrera force-pushed the service-instances-bindings branch 3 times, most recently from 6c9d67f to 02d8e1f Compare August 9, 2017 15:19

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2017

spadgett reviewed Aug 11, 2017

View reviewed changes

cdcabrera force-pushed the service-instances-bindings branch from 02d8e1f to 496cfdb Compare August 15, 2017 13:28

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 15, 2017

cdcabrera force-pushed the service-instances-bindings branch 5 times, most recently from 08c7767 to aafc1fa Compare August 17, 2017 06:22

spadgett reviewed Aug 17, 2017

View reviewed changes

cdcabrera force-pushed the service-instances-bindings branch 2 times, most recently from 55258ed to 99e5d5a Compare August 24, 2017 13:27

cdcabrera force-pushed the service-instances-bindings branch 2 times, most recently from e7ebf2d to 36b4f2f Compare August 25, 2017 18:13

cdcabrera force-pushed the service-instances-bindings branch from 76a42c1 to 5989859 Compare September 18, 2017 15:44

spadgett reviewed Sep 18, 2017

View reviewed changes

cdcabrera force-pushed the service-instances-bindings branch from 5989859 to 3248b3b Compare September 18, 2017 17:24

spadgett reviewed Sep 18, 2017

View reviewed changes

cdcabrera force-pushed the service-instances-bindings branch 2 times, most recently from 8e9be4a to fe5187b Compare September 18, 2017 18:33

spadgett changed the title ~~Prep for Service Instances & Bindings~~ Service Instance Details Pages Sep 18, 2017

spadgett approved these changes Sep 18, 2017

View reviewed changes

cdcabrera force-pushed the service-instances-bindings branch from fe5187b to 0fd1a62 Compare September 18, 2017 19:53

Updates for Service Instance & Bindings

f015611

Updates for service instances. Prep to allow updates to chosen parameters for instance and bindings.

cdcabrera force-pushed the service-instances-bindings branch from 0fd1a62 to f015611 Compare September 18, 2017 22:33

openshift-bot merged commit e1ba88d into openshift:master Sep 18, 2017

Service Instance Details Pages #1906

Service Instance Details Pages #1906

Conversation

cdcabrera commented Aug 3, 2017 • edited Loading

spadgett commented Aug 7, 2017

cdcabrera commented Aug 8, 2017 • edited Loading

spadgett 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

jeff-phillips-18 Aug 8, 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

Choose a reason for hiding this comment

cdcabrera commented Aug 17, 2017

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

cdcabrera Aug 25, 2017 • edited Loading

Choose a reason for hiding this comment

cdcabrera Aug 25, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett commented Aug 18, 2017

serenamarie125 commented Aug 18, 2017

spadgett commented Aug 21, 2017

beanh66 commented Aug 21, 2017

cdcabrera commented Aug 24, 2017

spadgett commented Sep 18, 2017

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

cdcabrera commented Sep 18, 2017

spadgett commented Sep 18, 2017

cdcabrera commented Sep 18, 2017

spadgett commented Sep 18, 2017

spadgett commented Sep 18, 2017

Choose a reason for hiding this comment

spadgett commented Sep 18, 2017

spadgett left a comment

Choose a reason for hiding this comment

spadgett commented Sep 18, 2017

spadgett commented Sep 18, 2017

spadgett commented Sep 18, 2017

spadgett commented Sep 18, 2017

cdcabrera commented Sep 18, 2017

openshift-bot commented Sep 18, 2017

spadgett commented Sep 18, 2017

openshift-bot commented Sep 18, 2017

openshift-bot commented Sep 18, 2017 • edited Loading

openshift-bot commented Sep 18, 2017

cdcabrera commented Aug 3, 2017 •

edited

Loading

cdcabrera commented Aug 8, 2017 •

edited

Loading

jeff-phillips-18 Aug 8, 2017 •

edited

Loading

cdcabrera Aug 25, 2017 •

edited

Loading

cdcabrera Aug 25, 2017 •

edited

Loading

openshift-bot commented Sep 18, 2017 •

edited

Loading