Label Filter for Kubernetes Deployment History Tab #1845

cdcabrera · 2017-07-13T20:25:17Z

Implementation of label filter for Kubernetes deployment history tab.

cdcabrera · 2017-07-13T20:28:40Z

@spadgett there appears to be a side issue where hitting refresh on the deployment view causes the filter recommendations to reset, is this something known? The refresh issue appears on both deployment views for Kubernetes and Deployment config

spadgett · 2017-07-14T00:38:34Z

there appears to be a side issue where hitting refresh on the deployment view causes the filter recommendations to reset, is this something known? The refresh issue appears on both deployment views for Kubernetes and Deployment config

There's some code that's supposed to handle it. We should try to debug why it's not working. You might start by setting a breakpoint there to see if the code is running.

origin-web-console/app/scripts/app.js

Lines 466 to 472 in d0e24f5

    
           .run(function($rootScope, LabelFilter){ 
        
             // assume we always want filterState persisted, pages that dont can turn it off 
        
             LabelFilter.persistFilterState(true); 
        
             $rootScope.$on('$routeChangeSuccess', function() { 
        
               LabelFilter.readPersistedState(); 
        
             }); 
        
           })

spadgett · 2017-07-14T00:39:47Z

app/scripts/controllers/deployment.js

@@ -21,11 +21,15 @@ angular.module('openshiftConsole')
                        OwnerReferencesService,
                        Logger,
                        ProjectsService,
-                        StorageService) {
+                        StorageService,
+                        LabelFilter) {


We've started to alphabetize the dependencies when possible to try and keep our sanity :) The lists can get long.

spadgett · 2017-07-14T00:40:54Z

app/scripts/controllers/deployment.js

+
+              $scope.unfilteredReplicaSetsForDeployment = DeploymentsService.sortByRevision(replicaSets);
+              $scope.replicaSetsForDeployment = LabelFilter.getLabelSelector().select($scope.unfilteredReplicaSetsForDeployment);
+              $scope.orderedReplicaSetsForDeployment = $scope.replicaSetsForDeployment;


I don't think we need both scope variables if they'll always have the same value. $scope.replicaSetsForDeployment seems OK.

spadgett · 2017-07-14T00:42:55Z

app/scripts/controllers/deployment.js

@@ -260,6 +269,28 @@ angular.module('openshiftConsole')
          Logger.log("builds (subscribe)", $scope.builds);
        }));

+        function updateFilterWarning() {
+          if (!LabelFilter.getLabelSelector().isEmpty() && $.isArray($scope.replicaSetsForDeployment) && !$scope.replicaSetsForDeployment.length && !$.isEmptyObject($scope.unfilteredReplicaSetsForDeployment)) {


Lodash makes it a little easier because you can pass an array or object to _.isEmpty

if (!LabelFilter.getLabelSelector().isEmpty() && _.isEmpty(scope.replicaSetsForDeployment) && !_.isEmpty($scope.unfilteredReplicaSetsForDeployment)) { // ... }

sure thing.

If we're looking for consistency, I could also flip the source to Lodash?

spadgett · 2017-07-14T00:44:00Z

app/scripts/controllers/deployment.js

@@ -260,6 +269,28 @@ angular.module('openshiftConsole')
          Logger.log("builds (subscribe)", $scope.builds);
        }));

+        function updateFilterWarning() {
+          if (!LabelFilter.getLabelSelector().isEmpty() && $.isArray($scope.replicaSetsForDeployment) && !$scope.replicaSetsForDeployment.length && !$.isEmptyObject($scope.unfilteredReplicaSetsForDeployment)) {
+            $scope.alerts["deployments"] = {


I'm sure this is what we used on the other page, but I suggest a more descriptive ID like

$scope.alerts['filter-hiding-all'] = ...

spadgett · 2017-07-14T00:45:17Z

app/scripts/controllers/deployment.js

+          if (!LabelFilter.getLabelSelector().isEmpty() && $.isArray($scope.replicaSetsForDeployment) && !$scope.replicaSetsForDeployment.length && !$.isEmptyObject($scope.unfilteredReplicaSetsForDeployment)) {
+            $scope.alerts["deployments"] = {
+              type: "warning",
+              details: "The active filters are hiding all deployments."


"...hiding all rollout history."

spadgett · 2017-07-14T00:45:48Z

app/scripts/controllers/deployment.js

+
+        LabelFilter.onActiveFiltersChanged(function(labelSelector) {
+          // trigger a digest loop
+          $scope.$apply(function() {


$scope.$evalAsync(function() { ... });

sure thing.

could also update the copy source on this one?

spadgett · 2017-07-14T00:47:24Z

app/views/browse/deployment.html

@@ -108,8 +110,11 @@ <h1 class="contains-actions">
                          <th>Created</th>
                        </tr>
                      </thead>
-                      <tbody>
-                        <tr ng-repeat="replicaSet in replicaSetsForDeployment">
+                      <tbody ng-if="(replicaSetsForDeployment | hashSize) == 0">


replicaSetsForDeployment is an array right? hashSize won't work on arrays. The size filter we have does, and we're trying to switch code to use it for that reason.

<tbody ng-if="!(replicaSetsForDeployment | size)">

spadgett · 2017-07-14T00:47:56Z

app/views/browse/deployment.html

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


size not hashSize

<tbody ng-if="replicaSetsForDeployment | size">

spadgett

LGTM except for the one comment. Do you mind opening a follow on issue for the filters not being saved on reload?

spadgett · 2017-07-14T12:24:09Z

app/scripts/controllers/deployment.js

-              $scope.replicaSetsForDeployment = DeploymentsService.sortByRevision(replicaSets);
+
+              $scope.unfilteredReplicaSetsForDeployment = DeploymentsService.sortByRevision(replicaSets);
+              $scope.replicaSetsForDeployment = LabelFilter.getLabelSelector().select($scope.unfilteredReplicaSetsForDeployment);


When we fix keeping filters on page reload, I think we need a call to updateFilterWarning here?

cdcabrera · 2017-07-14T15:02:53Z

The implementation of updateFilterWarning and two additional lines involving addLabelSuggestionsFromResources and setLabelSuggestions fixed the refresh issue for Kubernetes deployments. Additional review over the deployment config and I'm unable to recreate the refresh issue there. Looks like we can avoid declaring another issue

There is however a different refresh issue involving just the alerts directive on the Kubernetes deployments deployment.html, ever 3rd refresh and it double jumps. Any ideas?

spadgett · 2017-07-14T15:10:49Z

There is however a different refresh issue involving just the alerts directive on the Kubernetes deployments deployment.html, ever 3rd refresh and it double jumps. Any ideas?

I'm not clear on what happens. Do you have a screenshot or gif?

spadgett

Thanks @cdcabrera

spadgett · 2017-07-14T15:11:54Z

Approved for 3.7

cdcabrera · 2017-07-14T15:17:33Z

Refresh jump...

spadgett · 2017-07-14T15:43:16Z

@cdcabrera OK I see. Let's open a separate P3 issue

spadgett · 2017-07-19T14:13:16Z

We should hold this PR until #1836 and then rebase to pick up the new styles.

sg00dwin · 2017-07-19T18:01:12Z

@cdcabrera
fyi - in #1836 the class table-filter-wrapper was replaced with

https://github.com/sg00dwin/origin-web-console/blob/ddf4e78a77c0347c342ad1af577d8c2d1b09da36/app/views/browse/build-config.html#L178-L184

So that the filter input is wide.

spadgett · 2017-07-19T20:00:59Z

@cdcabrera FYI should be able to rebase now to pick up @sg00dwin's change

cdcabrera · 2017-07-19T20:51:26Z

Template is updated...
https://github.com/cdcabrera/origin-web-console/blob/f8fa1aa989691849ed42eefd2c51047b29c21b6b/app/views/browse/deployment.html#L95-L101

@spadgett @sg00dwin

spadgett · 2017-07-19T20:56:18Z

app/views/browse/deployment.html

@@ -92,7 +92,13 @@ <h1 class="contains-actions">
              <uib-tabset>
                <uib-tab active="selectedTab.history">
                  <uib-tab-heading>History</uib-tab-heading>
-                  <div ng-if="replicaSetsForDeployment | hashSize">
+                    <div class="table-filter-extension">


Looks like the indentation is off.

Implementation of label filter for Kubernetes deployment history tab

cdcabrera · 2017-07-21T14:11:01Z

[test]

spadgett · 2017-07-21T14:32:41Z

Error: Timed out waiting for the WebDriver server at http://172.18.5.25:33526/wd/hub

Flake #1684

cdcabrera · 2017-07-21T14:37:38Z

[test]

openshift-bot · 2017-07-21T14:39:09Z

Evaluated for origin web console test up to 5bbf8ab

openshift-bot · 2017-07-21T14:59:26Z

Origin Web Console Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_web_console/23/) (Base Commit: 7b414b3) (PR Branch Commit: 5bbf8ab)

spadgett · 2017-07-21T19:09:51Z

[merge][severity: bug]

openshift-bot · 2017-07-21T19:15:10Z

Evaluated for origin web console merge up to 5bbf8ab

openshift-bot · 2017-07-21T19:15:20Z

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/44/) (Base Commit: e839e15) (PR Branch Commit: 5bbf8ab) (Extended Tests: bug)

cdcabrera requested review from benjaminapetersen and spadgett July 13, 2017 20:25

spadgett requested changes Jul 14, 2017

View reviewed changes

cdcabrera force-pushed the issue-labelfilter branch from bbb61aa to 8116db3 Compare July 14, 2017 02:42

spadgett reviewed Jul 14, 2017

View reviewed changes

cdcabrera force-pushed the issue-labelfilter branch from 8116db3 to 2f3b018 Compare July 14, 2017 14:56

spadgett approved these changes Jul 14, 2017

View reviewed changes

cdcabrera force-pushed the issue-labelfilter branch from 2f3b018 to 7510a3f Compare July 17, 2017 16:25

cdcabrera mentioned this pull request Jul 17, 2017

Kubernetes deployment history tab alert jump on browser refresh #1857

Closed

cdcabrera force-pushed the issue-labelfilter branch from 7510a3f to f8fa1aa Compare July 19, 2017 20:46

cdcabrera requested a review from sg00dwin July 19, 2017 20:47

spadgett reviewed Jul 19, 2017

View reviewed changes

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

cdcabrera force-pushed the issue-labelfilter branch 2 times, most recently from 4cfded4 to 1361fdb Compare July 20, 2017 21:30

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

Label Filter for Kubernetes Deployment History Tab

5bbf8ab

Implementation of label filter for Kubernetes deployment history tab

cdcabrera force-pushed the issue-labelfilter branch from 1361fdb to 5bbf8ab Compare July 21, 2017 13:59

spadgett changed the title ~~[WIP] Label Filter for Kubernetes Deployment History Tab~~ Label Filter for Kubernetes Deployment History Tab Jul 21, 2017

openshift-bot merged commit ab5f706 into openshift:master Jul 21, 2017

Label Filter for Kubernetes Deployment History Tab #1845

Label Filter for Kubernetes Deployment History Tab #1845

Uh oh!

Conversation

cdcabrera commented Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdcabrera commented Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spadgett commented Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

spadgett Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

cdcabrera Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

spadgett left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdcabrera commented Jul 14, 2017

Uh oh!

spadgett commented Jul 14, 2017

Uh oh!

spadgett left a comment

Choose a reason for hiding this comment

Uh oh!

spadgett commented Jul 14, 2017

Uh oh!

cdcabrera commented Jul 14, 2017

Uh oh!

spadgett commented Jul 14, 2017

Uh oh!

spadgett commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sg00dwin commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

spadgett commented Jul 19, 2017

Uh oh!

cdcabrera commented Jul 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cdcabrera commented Jul 21, 2017

Uh oh!

spadgett commented Jul 21, 2017

Uh oh!

cdcabrera commented Jul 21, 2017

Uh oh!

openshift-bot commented Jul 21, 2017

Uh oh!

openshift-bot commented Jul 21, 2017

Uh oh!

cdcabrera commented Jul 13, 2017 •

edited

Loading

cdcabrera commented Jul 13, 2017 •

edited

Loading

spadgett commented Jul 14, 2017 •

edited

Loading

spadgett Jul 14, 2017 •

edited

Loading

cdcabrera Jul 14, 2017 •

edited

Loading

spadgett commented Jul 19, 2017 •

edited

Loading

sg00dwin commented Jul 19, 2017 •

edited

Loading

cdcabrera commented Jul 19, 2017 •

edited

Loading

openshift-bot commented Jul 21, 2017 •

edited

Loading