Fix the binding list update failure in service instance rows on the overview #1410

benjaminapetersen · 2017-04-10T17:47:40Z

$onChanges does not see properties of object row.state change
moving getBindings to $doCheck solves the problem
this does call the function on every digest loop, but it does not trigger infinite digests
- it is possible that storing a temp reference to the old bindings & testing against the new before assigning to row. instanceBindings could be slightly more performant as it would ensure no DOM updates, but that may be overkill in this situation.

benjaminapetersen · 2017-04-10T18:00:17Z

Eh, updated & added the check anyway. Better be safe.

jwforres · 2017-04-10T18:14:28Z

app/scripts/directives/overview/serviceInstanceRow.js

+  var prevBindings = [];
+  row.$doCheck = function() {
+    var bindings = getBindings();
+    if(prevBindings.length !== bindings.length) {


a length check isn't sufficient, you'll lose updates to the status of a binding

Instead of having to filter row.state.bindings every digest loop, can we keep a map of bindings by instance ref name updated in the overview watch callback? Then $doCheck might not be necessary.

@jwforres this just hits adding the additional binding row, I'm working on looking at the statuses now.

@spadgett i can do that, assuming that we would want something like this then:

<service-instance-row api-object="serviceInstance" bindings="bindingsByRef[serviceInstance.metadata.name]"></service-instance-row>

@benjaminapetersen that works for me and should perform better

&& overview.state.bindingsByInstanceRef, where the other maps seem to be.

benjaminapetersen · 2017-04-11T19:47:58Z

@jwforres @spadgett updated, this includes a couple filter additions that I'm using for other updates coming to the service instances.

benjaminapetersen · 2017-04-11T19:54:57Z

Fixes the following:

bindings will now update on overview after "bind" action
secret link will not appear until the binding action completes, will show "Pending"

spadgett · 2017-04-11T20:24:41Z

app/scripts/controllers/newOverview.js

@@ -1263,6 +1264,13 @@ function OverviewController($scope,
        resource: 'bindings'
      }, context, function(bindings) {
        state.bindings = bindings.by('metadata.name');
+        state.bindingsByInstanceRef = _.reduce(state.bindings, function(result, next) {


Can this just be

state.bindingsByInstanceRef = _.indexBy(state.bindings, 'spec.instanceRef.name');

spadgett · 2017-04-11T20:25:52Z

app/scripts/filters/resources.js

@@ -1093,6 +1105,13 @@ angular.module('openshiftConsole')
      return _.find(_.get(apiObject, 'status.conditions'), {type: type});
    };
  })
+  // returns true/false based on the presence of a Ready status conditions
+  // statusConditionReady(obj)
+  .filter('statusConditionReady', function(statusConditionFilter) {


Nit: I might give this a name to make it clear it's specific to service catalog items. Maybe isInstanceOrBindingReady since it doesn't apply to other types (a bit long I know)

Long, but works, I'll update.

You could also have two names for the filter. Call this isServiceInstanceReady and then have an isBindingReady filter that returns isServiceInstanceReadyFilter.

These might be useful in origin-web-common, but don't worry about that for this PR.

spadgett · 2017-04-11T20:27:42Z

app/views/new-overview.html

@@ -365,6 +365,7 @@ <h2 ng-if="overview.state.serviceInstances">
                  <service-instance-row
                    ng-repeat="serviceInstance in overview.state.orderedServiceInstances"
                    api-object="serviceInstance"
+                    bindings="overview.state.bindingsByInstanceRef[serviceInstance.metadata.name]"


Generally the items in state are meant to be accessed directly by the row. So maybe bindingsByInstanceRef should just be on overview directly.

But I'm OK either way.

Ok cool, I can move that.

spadgett · 2017-04-11T20:31:38Z

app/scripts/directives/overview/serviceInstanceRow.js

  };

  row.getSecretForBinding = function(binding) {
    return binding && _.get(row, ['state', 'secrets', binding.spec.secretName]);
  };

+  row.showSecretLink = function(binding) {
+    return statusConditionReady(binding);


I think it's more clear if you just call the filter directly in the view. I was confused why !showSecretLink() means show a pending icon, whereas

<span ng-if="!(binding | statusConditionReady)"> <status-icon status="'Pending'"></status-icon> Pending </span>

is pretty clear

Agree, I'll update. I originally had the filter logic in that function, factored it out to a filter but should have also made this change.

spadgett · 2017-04-11T20:32:03Z

app/views/overview/_service-instance-row.html

+          </span>
+          <a
+            ng-if="row.showSecretLink(binding)"
+            href="{{row.getSecretForBinding(binding) | navigateResourceURL}}">


Probably worth a canI check if you can view secrets

benjaminapetersen · 2017-04-12T03:14:20Z

@spadget i think i got all of the items, but doing a little more testing. Having trouble with my instances getting stuck.

spadgett · 2017-04-12T12:03:24Z

app/scripts/filters/resources.js

@@ -1093,6 +1104,16 @@ angular.module('openshiftConsole')
      return _.find(_.get(apiObject, 'status.conditions'), {type: type});
    };
  })
+  // returns true/false based on the presence of a Ready status conditions
+  // statusConditionReady(obj)


Looks like the comment has the old filter name.

spadgett · 2017-04-12T12:03:42Z

app/scripts/filters/resources.js

+      return _.get(statusConditionFilter(apiObject, 'Ready'), 'status') === 'True';
+    };
+  })
+  .filter('isBindingReady', function(isServiceInstanceReadyFilter) {


👍 I think these names read a lot better, thanks

spadgett · 2017-04-12T12:05:32Z

app/views/overview/_service-instance-row.html

+          </span>
+          <a
+            ng-if="binding | isBindingReady"
+            href="{{row.getSecretForBinding(binding) | navigateResourceURL}}">


Do you mind adding the canI check? Users with view access typically can't get secrets.

https://github.com/spadgett/origin-web-console/blob/a172029b6d375f52775f7b3d5b064f9a94c31fe4/app/views/_volumes.html#L22-L27

Yup, already did. let me push that up. Sorry, didn't update after a bit more tinkering last night.

spadgett · 2017-04-12T12:07:05Z

app/scripts/filters/resources.js

@@ -1084,7 +1084,18 @@ angular.module('openshiftConsole')
      return lastFinishTime;
    };
  })
+  // gets the first status condition.
  .filter('statusCondition', function() {


Hm, you can't change the behavior of this filter since it's used elsewhere (bind dialog)

Ill do a search again, but I thought I updated the other refrences. Let me check.

No I disagree with this behavior change. You may at any point have multiple status conditions, you need to actually ask for the one you want.

Ok, working on this.

statusConditionIs filter is the original code, so I aliased statusCondition back to it & updated my calls.

benjaminapetersen · 2017-04-12T14:08:02Z

Pushed again this morning, but give me a few, rebuilding my env as things blew up. I want to test a bit more before another review.

jwforres · 2017-04-12T17:22:08Z

app/views/overview/_service-instance-row.html

          <notification-icon ng-if="!row.expanded" alerts="row.notifications"></notification-icon>
+          <span ng-if="!row.isReady">
+            <status-icon status="row.status"></status-icon> {{row.statusReason | sentenceCase}}


it is unlikely status-icon is going to work with the condition reasons we are getting back, and at any time they might add more and break us / cause us to show the ? I would prefer we check for known failure conditions.

And this should probably be handled in the <notification-icon> above like other alerts, correct?

I'm opting for #2, the less verbose. Objections @jwforres @spadgett ?

spadgett · 2017-04-12T17:32:04Z

app/scripts/controllers/newOverview.js

@@ -1263,6 +1263,7 @@ function OverviewController($scope,
        resource: 'bindings'
      }, context, function(bindings) {
        state.bindings = bindings.by('metadata.name');
+        overview.bindingsByInstanceRef = _.indexBy(state.bindings, 'spec.instanceRef.name');


Should be _.groupBy not _.indexBy (my fault)

Heh. Was messing with that before I read this. Getting only 1 item every time was confusing 😄

jwforres · 2017-04-12T17:33:56Z

FYI I pulled down your PR and validated switching to _.groupBy works

…

On Wed, Apr 12, 2017 at 1:32 PM, Sam Padgett ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/scripts/controllers/newOverview.js <#1410 (comment)> : > @@ -1263,6 +1263,7 @@ function OverviewController($scope, resource: 'bindings' }, context, function(bindings) { state.bindings = bindings.by('metadata.name'); + overview.bindingsByInstanceRef = _.indexBy(state.bindings, 'spec.instanceRef.name'); Should be _.groupBy not _.indexBy (my fault) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1410 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZk7Uy_MwURPJlOGmoX-F9apxTFdoaYks5rvQqVgaJpZM4M5Gox> .

jwforres · 2017-04-12T18:44:58Z

Not sure why we would keep both in that case. Just keep statusCondition

…

On Wed, Apr 12, 2017 at 2:43 PM, Ben Petersen ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/scripts/filters/resources.js <#1410 (comment)> : > @@ -1084,7 +1084,18 @@ angular.module('openshiftConsole') return lastFinishTime; }; }) + // gets the first status condition. .filter('statusCondition', function() { statusConditionIs filter is the original code, so I aliased statusCondition back to it & updated my calls. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1410 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZk7U9hAjIgzhac39MO1-oR5-qU037tks5rvRtcgaJpZM4M5Gox> .

jwforres · 2017-04-12T19:04:53Z

If we are going to show the error from the API lets show the full error message, we don't typically truncate it

…

On Wed, Apr 12, 2017 at 2:30 PM, Ben Petersen ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In app/views/overview/_service-instance-row.html <#1410 (comment)> : > <notification-icon ng-if="!row.expanded" alerts="row.notifications"></notification-icon> + <span ng-if="!row.isReady"> + <status-icon status="row.status"></status-icon> {{row.statusReason | sentenceCase}} I'm opting for #2 <#2>, the less verbose. Objections @jwforres <https://github.com/jwforres> @spadgett <https://github.com/spadgett> ? [image: screen shot 2017-04-12 at 2 25 38 pm] <https://cloud.githubusercontent.com/assets/280512/24973378/7ca0835c-1f8c-11e7-9cac-e77fde3a3468.png> [image: screen shot 2017-04-12 at 2 29 34 pm] <https://cloud.githubusercontent.com/assets/280512/24973377/7c9fb9e0-1f8c-11e7-9a40-54686ed39271.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1410 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZk7cao5BsoDMxN1S9Ss4v4uFJbMyHsks5rvRg3gaJpZM4M5Gox> .

benjaminapetersen · 2017-04-12T19:09:10Z

Ok cool, got those.

benjaminapetersen · 2017-04-12T19:11:54Z

& rebased.

benjaminapetersen · 2017-04-12T19:44:55Z

one more tweak coming

jwforres · 2017-04-12T19:48:36Z

app/scripts/controllers/newOverview.js

@@ -1252,6 +1253,20 @@ function OverviewController($scope,
        resource: 'instances'
      }, context, function(serviceInstances) {
        state.serviceInstances = serviceInstances.by('metadata.name');
+        _.each(state.serviceInstances, function(instance) {
+          var failConditions = _.filter(instance.status.conditions, {status: 'False'});


a condition with status false is not necessarily a failure condition

benjaminapetersen · 2017-04-12T19:48:53Z

ok. think this is ready for one more pass?

jwforres · 2017-04-12T19:51:28Z

app/scripts/directives/overview/serviceInstanceRow.js

@@ -21,6 +22,8 @@ function ServiceInstanceRow($filter, DataService, rowMethods, $uibModal) {
  _.extend(row, rowMethods.ui);

  var getErrorDetails = $filter('getErrorDetails');
+  var statusCondition = $filter('statusCondition');


is anything actually referencing these?

benjaminapetersen · 2017-04-12T20:36:22Z

app/scripts/services/resourceAlerts.js

@@ -147,10 +147,46 @@ angular.module("openshiftConsole")
      return alerts;
    };



@jwforres @spadgett closer to the mark adding this?

updating to _.find since _.filter doesn't make sense here.

jwforres · 2017-04-12T20:49:42Z

[merge]

benjaminapetersen · 2017-04-12T20:51:12Z

😂

…verview

jwforres · 2017-04-12T21:07:21Z

[merge]

jwforres · 2017-04-12T22:48:26Z

Looks like a flake [merge]

…

On Apr 12, 2017 6:23 PM, "OpenShift Bot" ***@***.***> wrote: Origin Web Console Merge Results: FAILURE (https://ci.openshift.redhat. com/jenkins/job/test_pull_requests_origin_web_console/1222/) (Base Commit: aa4a228 <aa4a228> ) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1410 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZk7cgczNhSAz6UZ6kG86_Fm6lSoHErks5rvU7NgaJpZM4M5Gox> .

openshift-bot · 2017-04-12T22:55:09Z

Evaluated for origin web console merge up to 84fcfd6

openshift-bot · 2017-04-12T22:55:21Z

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

benjaminapetersen requested review from spadgett and jwforres April 10, 2017 17:47

benjaminapetersen force-pushed the binding-update branch from d6a966a to 2fc9393 Compare April 10, 2017 18:00

jwforres reviewed Apr 10, 2017

View reviewed changes

benjaminapetersen force-pushed the binding-update branch from 6dce473 to fd9a533 Compare April 11, 2017 19:46

spadgett requested changes Apr 11, 2017

View reviewed changes

benjaminapetersen force-pushed the binding-update branch 2 times, most recently from 525af44 to 10e91cb Compare April 12, 2017 03:10

spadgett requested changes Apr 12, 2017

View reviewed changes

benjaminapetersen force-pushed the binding-update branch from 10e91cb to cf21a96 Compare April 12, 2017 13:28

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

jwforres reviewed Apr 12, 2017

View reviewed changes

spadgett reviewed Apr 12, 2017

View reviewed changes

benjaminapetersen force-pushed the binding-update branch from cf21a96 to fd8e795 Compare April 12, 2017 18:53

benjaminapetersen force-pushed the binding-update branch from fd8e795 to 12a57cf Compare April 12, 2017 19:08

benjaminapetersen force-pushed the binding-update branch from 12a57cf to 214d770 Compare April 12, 2017 19:11

benjaminapetersen force-pushed the binding-update branch from 214d770 to d42602a Compare April 12, 2017 19:48

jwforres reviewed Apr 12, 2017

View reviewed changes

benjaminapetersen force-pushed the binding-update branch from d42602a to d3293b3 Compare April 12, 2017 20:35

benjaminapetersen commented Apr 12, 2017

View reviewed changes

jwforres approved these changes Apr 12, 2017

View reviewed changes

benjaminapetersen force-pushed the binding-update branch from d3293b3 to 4fec96c Compare April 12, 2017 20:46

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

Fix the binding list update failure in service instance rows on the o…

84fcfd6

…verview

benjaminapetersen force-pushed the binding-update branch from 4fec96c to 84fcfd6 Compare April 12, 2017 20:59

openshift-bot merged commit dd72cfb into openshift:master Apr 12, 2017

		@@ -147,10 +147,46 @@ angular.module("openshiftConsole")
		return alerts;
		};

Fix the binding list update failure in service instance rows on the overview #1410

Fix the binding list update failure in service instance rows on the overview #1410

Uh oh!

Conversation

benjaminapetersen commented Apr 10, 2017

Uh oh!

benjaminapetersen commented Apr 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjaminapetersen commented Apr 11, 2017

Uh oh!

benjaminapetersen commented Apr 11, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjaminapetersen commented Apr 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjaminapetersen commented Apr 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwforres commented Apr 12, 2017 via email

Uh oh!

jwforres commented Apr 12, 2017 via email

Uh oh!

jwforres commented Apr 12, 2017 via email

Uh oh!

benjaminapetersen commented Apr 12, 2017

Uh oh!

openshift-bot commented Apr 12, 2017 •

edited

Loading