Display Lifecycle hooks on the DC browse page #966

jhadvig · 2016-12-01T15:32:25Z

I've created 3 previews on how we could display lifecycle hooks on the DC browse page. All three previews have similar structure, they only differ by the section where the particular hook description is displayed and the position of the action that the hook is doing, which is either right from the hook type, or under the hook type (text-muted class)

As part of the Configuration section
Separate section in the Details tab

Here I was not sure where to put the section so I've put it under the Triggers section
A separate Lifecycle Hooks tab since as you can see on the previous screens the Details tab can get pretty messy whe it contains a lot of things.

IMHO I like the last option with the new tab, since it makes things more clean&clear. But I also understand that the hooks are more or less part of a "Configuration". On the other hand I kinda like the idea to split the some sections of the Details tab into a standalone tabs... eg: Autoscaling or Triggers ?!?

@jwforres @spadgett please let me know what what do you think. Please dont review the code for now ..was just prototyping. Thanks :)

spadgett · 2016-12-01T15:44:05Z

I like having the separate tab. Shorten it to just "Hooks"?

I'm not sure about using the pod template styling for hooks. We currently use it as a distinct visual style for pod templates. Maybe something closer to the styling we use for dc details.

Otherwise looks good!

jhadvig · 2016-12-01T16:03:18Z

yeah regarding similar styling that we have for the pod template.. I've reused it since it's using the concept of separating the details per container and thats basically what the lifecycle hooks do.

jwforres · 2016-12-01T16:15:58Z

Honestly I prefer option 2. I think if we were going to split stuff into another tab, we should really take a look at everything we have on that Details tab and come up with a logical split. Hooks as its own tab doesn't make sense to me. I'd expect a split more like (not necessarily this order):

How am I deploying:

Strategy and params
Hooks
Scaling / Autoscaling
Triggers

What am I deploying:

Pod template details
Volumes

jhadvig · 2016-12-01T17:04:23Z

@spadgett @jwforres here is preview of the styling we have in dc details in a separate tab:

Here is preview of the styling we have in dc details in the Details tab:

jhadvig · 2016-12-01T17:07:21Z

I liked the the pod-template like styling more, but dont have any major objection against the styling in the last comment as well (exepct #966 (comment))

jhadvig · 2016-12-02T16:07:11Z

I've updated the PR based on the conversation with @spadgett ..
Here is the screen on the how the Hooks will be displayed:

Also had to refactor the lifecycleHook directive

PTAL

Will add the dist after review

spadgett · 2016-12-05T14:00:07Z

app/scripts/filters/resources.js

+      if (!strategyName) {
+        return null;
+      }
+      return strategyName.toLowerCase() + 'Params';


We shouldn't assume this will always work. I'd rather have a switch statement for all the strategies we know about.

spadgett · 2016-12-05T14:02:14Z

app/views/browse/deployment-config.html

-                              <dt>Max Surge:</dt>
-                              <dd>{{deploymentConfig.spec.strategy.rollingParams.maxSurge}}</dd>
+                              <dd>{{deploymentConfig.spec.strategy[strategyParams].timeoutSeconds}} sec</dd>
+                              <div ng-if="deploymentConfig.spec.strategy.rollingParams">


While you're in the code, can you change this to...?

<dt ng-if-start="deploymentConfig.spec.strategy.rollingParams">Update Period:</dt> <dd>{{deploymentConfig.spec.strategy.rollingParams.updatePeriodSeconds}} sec</dd> ... <dt>Max Surge:</dt> <dd ng-if-end>{{deploymentConfig.spec.strategy.rollingParams.maxSurge}}</dd>

Then you don't have the unnecessary div in the middle of the dl.

spadgett · 2016-12-05T14:14:27Z

app/views/browse/deployment-config.html

+                        <div class="col-lg-6" ng-if="deploymentConfig.spec.strategy.type !== 'Custom'">
+                          <h3>
+                            Hooks
+                            <a href="{{'lifecycle_hooks' | helpLink}}" style="display: inline-block;" target="_blank">


Let's not use inline styles. Does class="learn-more-inline" work here?

spadgett · 2016-12-05T14:18:37Z

app/views/browse/deployment-config.html

+                          <h3>
+                            Hooks
+                            <a href="{{'lifecycle_hooks' | helpLink}}" style="display: inline-block;" target="_blank">
+                              <span class="learn-more-block">Learn more <i class="fa fa-external-link"> </i></span>


Should be using learn-more-inline not learn-more-block.

Can you try

<span class="learn-more-inline"> <a ng-href="{{'lifecycle_hooks' | helpLink}}" target="_blank">Learn more <i class="fa fa-external-link" aria-hidden="true"></i></a> </span>

spadgett · 2016-12-05T14:32:14Z

app/scripts/directives/lifecycleHook.js

+      scope: {
+        hookParams: "=",
+        namespace: "=",
+        type: "@"


I think this is better as

scope: { deploymentConfig: '=', type: '@' // "pre", "mid", or "post" }

Should be able to determine strategy from the deployment config spec, correct? Then you don't need to pass namespace because it's in the deployment config metadata.

spadgett · 2016-12-05T14:38:41Z

app/views/directives/lifecycle-hook.html

+          <dt>Environment Variables:</dt>
+          <dd>
+            <div ng-repeat="env in hookParams.execNewPod.env">
+              {{env.name}}&#8201;&#8594;&#8201;{{env.value}}


I think {{env.name}}={{env.value}} is more natural for environment variables.

spadgett · 2016-12-05T14:41:29Z

app/views/directives/lifecycle-hook.html

+  <div ng-if="hookParams.execNewPod">
+    <h5 class="container-name">Container {{hookParams.execNewPod.containerName}}</h5>
+    <dl class="dl-horizontal left">
+      <div class="is-item-description">


I don't see where this CSS class is defined.

spadgett · 2016-12-05T14:41:35Z

app/views/directives/lifecycle-hook.html

+    <div ng-repeat="tagImage in hookParams.tagImages">
+    <h5 class="container-name">Container {{tagImage.containerName}}</h5>
+      <dl class="dl-horizontal left">
+        <div class="is-item-description">


I don't see where this CSS class is defined.

spadgett · 2016-12-05T14:43:37Z

app/views/directives/lifecycle-hook.html

-              <istag-select model="istagHook" allow-custom-tag="true" select-disabled="view.isDisabled"></istag-select>
+<h4>
+  {{type}} Hook
+  <small class="text-muted">{{hookParams.execNewPod ? "Runs a command in a new pod based on the specified container" : "Tags image on deployment success"}}</small>


Same comment as failure policy: I don't think we should style this as help text because it's a configuration value that changes. I'd rather describe the pre/mid/post hook type in help text, then show execNewPod and failurePolicy clearly as configuration below the header.

spadgett · 2016-12-05T14:45:50Z

app/views/directives/lifecycle-hook.html

+        <div class="is-item-description">
+          <dt>Tag As:</dt>
+          <dd>
+            <a ng-href="{{tagImage.to.name | navigateResourceURL : 'ImageStreamTag' : namespace}}">


Do you need to check first for tagImage.to.namespace? Then fall back namespace if not defined. (If tagImage.to.namespace && tagImage.to.namespace !== namespace, it should not be a link.)

jhadvig · 2016-12-06T10:26:28Z

@spadgett I've updated the PR based on your comments.

Here is the screen of the Hooks in the DC browse page:

Wasnt really sure how to properly name the execNewPod and tagImages actions, so I named it Action and put its description next to it.
Also added a tooltip to the failurePolicy to explain what the value does.

PTAL

spadgett · 2016-12-06T13:37:06Z

app/scripts/directives/lifecycleHook.js

+      },
+      templateUrl: 'views/directives/lifecycle-hook.html',
+      controller: function($scope) {
+        $scope.strategyParams = $filter('deploymentStrategyNameToParams')($scope.deploymentConfig.spec.strategy.type);


You should watch the type in case it changes.

$scope.$watch('deploymentConfig.spec.strategy.type', function(type) { $scope.strategyParams = $filter('deploymentStrategyNameToParams')(type); });

spadgett · 2016-12-06T13:41:12Z

app/scripts/filters/resources.js

@@ -1012,6 +1012,20 @@ angular.module('openshiftConsole')
      return 'PAUSED_PENDING_INPUT' === stage.status;
    };
  })
+  .filter('deploymentStrategyNameToParams', function () {


I'd rather the filter give me the strategy params instead of the property name.

.filter('strategyParams', function() { return function(deploymentConfig) { var strategy = _.get(deploymentConfig, 'spec.strategy.type'); switch (strategy) { case 'Recreate': return _.get(deploymentConfig, 'spec.strategy.recreateParams', {}); // ... default: return null; } }; });

Then in the directive link function, you can get the strategy params with

$scope.$watch('deploymentConfig', function(deploymentConfig) { $scope.strategyParams = $filter('strategyParams')(deploymentConfig); });

(Should have made that comment on first review, sorry.)

spadgett · 2016-12-06T13:42:00Z

app/styles/_components.less

@@ -188,7 +188,7 @@
  }
 }

-code.probe-command {
+code.command {


spadgett · 2016-12-06T13:46:33Z

app/views/browse/deployment-config.html

+                          <h3>
+                            Hooks
+                            <span class="learn-more-inline">
+                              <a ng-href="{{'lifecycle_hooks' | helpLink}}" target="_blank">Learn more&nbsp;<i class="fa fa-external-link" aria-hidden="true"></i></a>


Should be "Learn More" (uppercase M) now. Changed the doc links in #978.

spadgett · 2016-12-06T13:48:08Z

app/views/directives/lifecycle-hook.html

+<span ng-switch="type">
+  <small class="text-muted" ng-switch-when="pre">Pre hooks execute before the deployment begins.</small>
+  <small class="text-muted" ng-switch-when="mid">Mid hooks execute after the previous deployment is scaled down to zero and before the first pod of the new deployment is created.</small>
+  <small class="text-muted" ng-switch-when="post">Post hooks execute after the deployment strategy completes.</small>


Instead of small, let's use the help-block style. (Margin looked off in the screenshot, but I think help-block will fix that.)

spadgett · 2016-12-06T13:53:55Z

app/views/directives/lifecycle-hook.html

+</span>
+<dl class="dl-horizontal left">
+  <dt>Action:</dt>
+  <dd>{{deploymentConfig.spec.strategy[strategyParams][type].execNewPod ? "Runs a command in a new pod based on the specified container" : "Tags image on deployment success"}}</dd>


I'd probably shorten these to "Run a command" and "Tag the image"

spadgett · 2016-12-06T13:57:03Z

app/views/directives/lifecycle-hook.html

+    <div ng-repeat="tagImage in deploymentConfig.spec.strategy[strategyParams][type].tagImages">
+    <h5 class="container-name">Container {{tagImage.containerName}}</h5>
+      <dl class="dl-horizontal left">
+        <dt>Tag As:</dt>


If there is only ever one item in this dl, I'd just put it in a div.

<div> Tag image as <a href=...>myproject/image:latest</a> </div>

jhadvig · 2016-12-06T15:26:55Z

@spadgett comments addressed

jhadvig · 2016-12-06T16:02:10Z

Adding screen:

wasnt really sure what limit values shall be set for the envVars so I set the name to 40 and the value to 80. Thoughts ?

spadgett · 2016-12-06T16:19:14Z

app/views/directives/lifecycle-hook.html

+  <span ng-switch="type">
+    <small ng-switch-when="pre">&ndash; runs before the deployment begins.</small>
+    <small ng-switch-when="mid">&ndash; runs after the previous deployment is scaled down to zero and before the first pod of the new deployment is created.</small>
+    <small ng-switch-when="post">&ndash; runs after the deployment strategy completes.</small>


nit: remove the period from the end of these messages

spadgett · 2016-12-06T16:21:54Z

Instead of truncate long text, does class="truncate" work? Since it's a single line, I prefer to use CSS. Then it will truncate exactly at the edge of the screen.

spadgett · 2016-12-06T16:22:58Z

If we switch to class="truncate", set a title attribute with the full value as well.

jhadvig · 2016-12-07T13:20:38Z

@spadgett updated :)
added the dist as well
PTAL

spadgett · 2016-12-07T14:43:20Z

[merge]

openshift-bot · 2016-12-07T14:47:34Z

Evaluated for origin web console merge up to 8d402e9

openshift-bot · 2016-12-07T14:47:44Z

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

jhadvig force-pushed the dc_details branch from e65b5f1 to 3809b25 Compare December 2, 2016 16:01

spadgett requested changes Dec 5, 2016

View reviewed changes

openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 5, 2016

spadgett self-assigned this Dec 5, 2016

jhadvig force-pushed the dc_details branch from 3809b25 to 6bf41fd Compare December 6, 2016 10:22

openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 6, 2016

spadgett requested changes Dec 6, 2016

View reviewed changes

jhadvig force-pushed the dc_details branch from 6bf41fd to 598dd39 Compare December 6, 2016 15:26

jhadvig force-pushed the dc_details branch 2 times, most recently from 896b559 to dda4522 Compare December 6, 2016 16:01

spadgett reviewed Dec 6, 2016

View reviewed changes

Display Lifecycle hooks on the DC browse page

8d402e9

jhadvig force-pushed the dc_details branch from dda4522 to 8d402e9 Compare December 7, 2016 13:20

spadgett approved these changes Dec 7, 2016

View reviewed changes

openshift-bot merged commit 8fd9c66 into openshift:master Dec 7, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Display Lifecycle hooks on the DC browse page #966

Display Lifecycle hooks on the DC browse page #966

jhadvig commented Dec 1, 2016

spadgett commented Dec 1, 2016

jhadvig commented Dec 1, 2016

jwforres commented Dec 1, 2016

jhadvig commented Dec 1, 2016

jhadvig commented Dec 1, 2016

jhadvig commented Dec 2, 2016 •

edited

spadgett Dec 5, 2016

spadgett Dec 5, 2016

spadgett Dec 5, 2016

spadgett Dec 5, 2016

spadgett Dec 5, 2016

spadgett Dec 5, 2016

spadgett Dec 5, 2016

spadgett Dec 5, 2016

spadgett Dec 5, 2016

spadgett Dec 5, 2016

jhadvig commented Dec 6, 2016

spadgett Dec 6, 2016

spadgett Dec 6, 2016

spadgett Dec 6, 2016

spadgett Dec 6, 2016

spadgett Dec 6, 2016

spadgett Dec 6, 2016

spadgett Dec 6, 2016

jhadvig commented Dec 6, 2016

jhadvig commented Dec 6, 2016 •

edited

spadgett Dec 6, 2016

spadgett commented Dec 6, 2016

spadgett commented Dec 6, 2016

jhadvig commented Dec 7, 2016

spadgett commented Dec 7, 2016

openshift-bot commented Dec 7, 2016

openshift-bot commented Dec 7, 2016 •

edited

Display Lifecycle hooks on the DC browse page #966

Display Lifecycle hooks on the DC browse page #966

Conversation

jhadvig commented Dec 1, 2016

spadgett commented Dec 1, 2016

jhadvig commented Dec 1, 2016

jwforres commented Dec 1, 2016

jhadvig commented Dec 1, 2016

jhadvig commented Dec 1, 2016

jhadvig commented Dec 2, 2016 • edited

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

jhadvig commented Dec 6, 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

jhadvig commented Dec 6, 2016

jhadvig commented Dec 6, 2016 • edited

Choose a reason for hiding this comment

spadgett commented Dec 6, 2016

spadgett commented Dec 6, 2016

jhadvig commented Dec 7, 2016

spadgett commented Dec 7, 2016

openshift-bot commented Dec 7, 2016

openshift-bot commented Dec 7, 2016 • edited

jhadvig commented Dec 2, 2016 •

edited

jhadvig commented Dec 6, 2016 •

edited

openshift-bot commented Dec 7, 2016 •

edited