Init Containers #1560

cdcabrera · 2017-05-19T19:08:00Z

Update for displaying init containers... includes

status.initContainerStatuses
spec.initContainers

spadgett · 2017-05-19T19:16:46Z

@cdcabrera Let's show the container statuses on the left in the same order as the spec on the right (spec order should win). Let me know when you're ready for code review.

cdcabrera · 2017-05-20T06:58:07Z

Good catch on the sorting order, can do. And yes indeed on the code review

spadgett

Thanks @cdcabrera.

I feel like visually it's not immediately obvious which containers are init containers. You have to look closely at the label, which is small text. Maybe we do need a separate heading. (I know you implemented what I suggested.)

I also think we should collapse init containers under status when they're complete and show a summary and a details link.

We should check the logic for debug terminals. I think it'll need changes.

spadgett · 2017-05-22T16:14:07Z

app/scripts/directives/resources.js

+      restrict: 'E',
+      scope: {
+        podStatus: '=',
+        initStatus: '=?',


We should decide if we want one directive for all statuses or a directive for each individual container status or (or both). I'm OK either way. If we have one directive, I'd call it containerStatuses since it doesn't show all of pod status, just the status of the containers.

If we have one directive, I'd remove initStatus and just always show it. @jwforres and I talked about maybe collapsing the init container status with a summary when the init containers are complete. @cdcabrera let's discuss

spadgett · 2017-05-22T16:16:59Z

app/scripts/directives/resources.js

+      scope: {
+        podStatus: '=',
+        initStatus: '=?',
+        terminal: '=?'


I'd give this a more descriptive name like startDebugTerminal

spadgett · 2017-05-22T16:27:18Z

app/scripts/directives/resources.js

+      },
+      templateUrl: 'views/_pod-status.html',
+      link: function(scope) {
+        scope.isTerminal = angular.isFunction(scope.terminal);


I feel like isTerminal is slightly confusing since the directive itself is not a terminal. Maybe hasDebugTerminal or enableDebugTerminal?

spadgett · 2017-05-22T16:29:51Z

app/scripts/directives/resources.js

@@ -24,10 +71,18 @@ angular.module('openshiftConsole')
        imagesByDockerReference: '=',
        builds: '=',
        detailed: '=?',
+        initContainers: '=?',


Same comment here. I'd remove initContainers, have one pod-template directive, and just show init containers when detailed is enabled. This means you'll need to pull the contents from the ng-repeat in the view for a container into a separate directive, maybe pod-template-container.

Appears the property detailed is also used for the health checks if missing info alert... adjusting the initContainers as part of details now affects the banner display as well.

spadgett · 2017-05-22T16:32:28Z

app/scripts/directives/resources.js

-      templateUrl: 'views/_pod-template.html'
+      templateUrl: 'views/_pod-template.html',
+      link: function(scope) {
+        scope.podTemplateContainers = _.get(scope.podTemplate, 'spec.containers', []);


Suggest scope.initContainers and scope.containers as variable names

spadgett · 2017-05-22T16:33:32Z

app/scripts/controllers/pod.js

@@ -419,7 +419,7 @@ angular.module('openshiftConsole')
          return running;
        };

-        $scope.showDebugAction = function(containerStatus) {
+        /*$scope.showDebugAction = function(containerStatus) {


When finished, don't forget to remove the commented out code.

benjaminapetersen · 2017-05-23T19:22:02Z

app/views/_container-statuses.html

@@ -0,0 +1,48 @@
+<!--


Curious about these comments here, these variables are not in the directive scope?

Caught podStatus earlier, should be updated now... if we're going for only listing values/properties/variables used in the templates can adjust accordingly

benjaminapetersen · 2017-05-23T19:23:21Z

app/views/_pod-template-container.html

@@ -0,0 +1,188 @@
+<!--


Same as above re:scope.

Caught initPrefix earlier, should be updated now...

cdcabrera · 2017-05-24T15:21:47Z

@spadgett links for hide/show on init container status added... originally planned to have single links associated with all init container statuses. Checked in individual links, as it was starting to appear like a title/label was going to be needed to break them apart, see screen shots... but can update towards grouped init container statuses

spadgett · 2017-05-24T17:20:55Z

I was envisioning something like

✓ Init container my-init-container completed succesfully. <a href="">Show Details</a>

or

✓ 2 init containers completed successfully. <a href="">Show Details</a>

in normal text with a green checkmark. Most of the time when they run and complete you don't care too much, so they shouldn't take as much focus.

spadgett · 2017-05-24T17:24:07Z

"Init Template" isn't really right. It's only one template. I'd rather leave the "Template" heading and have subheadings "Init Containers" and "Containers" in the same weight as "Volumes" just below.

cdcabrera · 2017-05-25T19:19:07Z

@spadgett updated screen captures for init container status... icon and some styling adjustments probably need to take place

spadgett · 2017-05-25T19:20:48Z

app/scripts/directives/resources.js

+        scope.podContainerStatuses = _.get(scope.containerStatuses, 'status.containerStatuses', []);
+        scope.podContainerInitStatuses = _.get(scope.containerStatuses, 'status.initContainerStatuses', []);
+
+        scope.hasContainerFailed = function (containerStatus) {


Shouldn't need to make this a scope var if it's not used in the view. Suggest just using

var isContainerFailed = $filter('isContainerFailed');

then below calling isContainerFailed()

spadgett · 2017-05-25T19:22:57Z

app/scripts/directives/resources.js

+          return $filter('isContainerFailed')(containerStatus) !== false;
+        };
+
+        scope.haveContainersFailed = function(containerStatuses) {


Same comment here. Doesn't need to be a scope function. _.some makes this easier.

var isContainerFailed = $filter('isContainerFailed'); var haveContainersFailed = function(containerStatuses) { return _.some(containerStatuses, isContainerFailed); };

https://lodash.com/docs/3.10.1#some

spadgett · 2017-05-25T19:26:16Z

app/scripts/directives/resources.js

+    return {
+      restrict: 'E',
+      scope: {
+        containerStatuses: '=',


Let's rename this pod since it's taking in a pod, not the containerStatuses array

spadgett · 2017-05-25T19:27:30Z

app/scripts/directives/resources.js

+          return true;
+        };
+
+        scope.expandInitContainers = scope.initContainersFailed = scope.haveContainersFailed(scope.podContainerInitStatuses);


This won't update when the pod status changes. I'd expect the section to automatically collapse if the init containers complete when you're on the page.

spadgett · 2017-05-25T19:29:56Z

app/scripts/directives/resources.js

+      templateUrl: 'views/_pod-template.html',
+      link: function(scope) {
+        scope.containers = _.get(scope.podTemplate, 'spec.containers', []);
+        scope.initContainers = _.get(scope.podTemplate, 'spec.initContainers', []);


If we do this assignment here, it won't update when the pod status changes. Easier just to access podTemplate.spec.containers or podTemplate.spec.initContainers directly in the view.

spadgett · 2017-05-25T19:47:05Z

app/views/_pod-template.html

-        </div>
-      </div>
+  <!-- Prompt to add health checks if missing. -->
+  <div ng-if="detailed && addHealthCheckUrl && !(podTemplate | hasHealthChecks)" class="alert alert-info">


We shouldn't have the same health check warning twice.

spadgett · 2017-05-25T19:47:37Z

app/views/_pod-template.html

-        </div>
-      </div>
+  <div class="pod-template-container">
+    <div class="pod-template-block" ng-repeat="container in containers | orderBy:'name'">


Remove orderBy

Sure thing, related to comment above regarding display order for status containers.

spadgett · 2017-05-25T19:48:21Z

app/views/_pod-template.html


      <div
        extension-point
        extension-name="container-links"
        extension-types="link dom"
-        extension-args="[container, podTemplate]"></div>
+        extension-args="[container, containers]"></div>


This can't change as it might break extensions. We should still pass podTemplate as the second arg.

spadgett · 2017-05-25T19:50:04Z

app/views/_pod-template.html

+        extension-point
+        extension-name="container-links"
+        extension-types="link dom"
+        extension-args="[container, initContainers]"></div>


Hm, can't decide if we want / need the extension point for init containers. There's no way for the extension to know that it's an init container, so I'm inclined to leave it out.

@jwforres

spadgett · 2017-05-25T19:50:58Z

app/views/_pod-template-container.html

+ -->
+
+<div class="pod-template">
+  <div class="component-label"><span ng-bind-template="{{labelPrefix}}:"></span> {{container.name}}</div>


Suggest just defaulting to "Container:" if no labelPrefix was given.

spadgett

Thanks @cdcabrera, a few more comments.

spadgett · 2017-06-02T18:07:12Z

app/scripts/directives/resources.js

+      link: function(scope) {
+        scope.hasDebugTerminal = angular.isFunction(scope.startDebugTerminal);
+
+        var isContainerTerminated = $filter('isContainerTerminatedSuccessfully');


I'd keep Successfully in the name since this should return false if it terminated with a failure.

spadgett · 2017-06-02T18:07:53Z

app/scripts/directives/resources.js

+        scope.hasDebugTerminal = angular.isFunction(scope.startDebugTerminal);
+
+        var isContainerTerminated = $filter('isContainerTerminatedSuccessfully');
+        var haveContainersTerminated = function(containerStatuses) {


Then maybe haveAllContainersTerminatedSuccessfully. (A little long maybe, but precise.)

spadgett · 2017-06-02T18:08:27Z

app/scripts/directives/resources.js

+
+        var isContainerTerminated = $filter('isContainerTerminatedSuccessfully');
+        var haveContainersTerminated = function(containerStatuses) {
+          return _.some(containerStatuses, isContainerTerminated);


Should this be _.every? We only want to collapse when they've all terminated.

spadgett · 2017-06-02T18:20:21Z

app/views/_container-statuses.html

+    <span flex>
+      <span ng-if="initContainersTerminated"> &nbsp;{{pod.status.initContainerStatuses.length}}</span>
+      <ng-pluralize count="pod.status.initContainerStatuses.length"
+                    when="{'1': 'Init Container','other': 'Init Containers'}">


It might be good to use the container name when there's one.

Init container my-init-container completed successfully

or if more than one like you have

3 init containers completed successfully

We usually don't capitalize types in sentences.

spadgett · 2017-06-02T18:23:08Z

app/views/_container-statuses.html

+    </span>
+    <span ng-if="initContainersTerminated">
+      <a class="page-header-link" href="" ng-click="toggleInitContainer()" ng-if="!expandInitContainers">Show Details</a>
+      <a class="page-header-link" href="" ng-click="toggleInitContainer()" ng-if="expandInitContainers">Hide Details</a>


Suggest one a element and just change the message inside since everything else is the same.

<a class="page-header-link" href="" ng-click="toggleInitContainer()"> <span ng-if="!expandInitContainers">Show</span> <span ng-if="expandInitContainers">Hide</span> Details </a>

spadgett · 2017-06-02T18:54:11Z

app/views/_pod-template.html

-  <span ng-if="podTemplate.spec.containers.length === 1">This container has no health checks</span>
-  <span ng-if="podTemplate.spec.containers.length > 1">Not all containers have health checks</span>
+  <span ng-if="podTemplate.spec.initContainers.length === 1">This container has no health checks</span>
+  <span ng-if="podTemplate.spec.initContainers.length > 1">Not all containers have health checks</span>


Init containers do not have health checks. We should still check spec.containers.length here.

Maybe change This container to Container <name> does not... if there is only one container. Then it's clear "This container" does not refer to the init container.

giving it a shot

spadgett · 2017-06-02T18:55:04Z

app/scripts/directives/resources.js

+      restrict: 'E',
+      scope: {
+        pod: '=',
+        startDebugTerminal: '=?',


I know I suggested this name, but I think maybe onDebugTerminal is better :) sorry

no worries, can do

spadgett · 2017-06-06T12:32:18Z

@cdcabrera I noticed the style for container status is different than regular containers. We should be consistent:

cdcabrera · 2017-06-09T19:19:38Z

Restyled for consistency, reduced the need for nested labels. @openshift/team-ux-review @spadgett

Multiple init containers

And a single init container

spadgett · 2017-06-09T19:46:15Z

app/views/_container-statuses.html

+
+  <div class="animate-if"
+       ng-if="expandInitContainers"
+       ng-repeat="containerStatus in pod.status.initContainerStatuses" >


To avoid flicker:

ng-repeat="containerStatus in pod.status.initContainerStatuses track by containerStatus.name">

spadgett · 2017-06-09T19:46:29Z

app/views/_container-statuses.html

+  </div>
+</div>
+
+<div ng-repeat="containerStatus in pod.status.containerStatuses" >


ng-repeat="containerStatus in pod.status.containerStatuses track by containerStatus.name">

spadgett

Sorry @cdcabrera missed a couple things on previous review :/

spadgett · 2017-06-13T15:26:27Z

app/scripts/directives/resources.js

+
+        scope.showDebugAction = function (containerStatus) {
+
+          if (_.get(scope.containerStatuses, 'status.phase') === 'Completed') {


_.get(scope.pod, 'status.phase') === 'Completed'

Can do, when updated the scope property the old search and replace missed a couple

spadgett · 2017-06-13T15:26:54Z

app/scripts/directives/resources.js

+            return false;
+          }
+
+          if ($filter('annotation')(scope.containerStatuses, 'openshift.io/build.name')) {


$filter('annotation')(scope.pod, 'openshift.io/build.name')

spadgett · 2017-06-13T15:27:16Z

app/scripts/directives/resources.js

+            return false;
+          }
+
+          if ($filter('isDebugPod')(scope.containerStatuses)) {


$filter('isDebugPod')(scope.pod))

spadgett · 2017-06-13T19:58:21Z

@cdcabrera Sorry found one more bug... :(

Init containers header showing up when there are none:

spadgett · 2017-06-13T20:00:07Z

app/views/_pod-template.html

-          </div>
-        </div>
-      </div>
+<div ng-if="detailed">


Update for pod template directive to allow displaying init containers

spadgett · 2017-06-13T20:44:11Z

[merge]

openshift-bot · 2017-06-13T20:47:27Z

Evaluated for origin web console merge up to fef9211

openshift-bot · 2017-06-13T20:47:28Z

[Test]ing while waiting on the merge queue

openshift-bot · 2017-06-13T21:07:13Z

Evaluated for origin web console test up to fef9211

openshift-bot · 2017-06-13T21:47:02Z

Origin Web Console Test Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_web_console/1522/) (Base Commit: 50bfbcc)

openshift-bot · 2017-06-14T00:07:21Z

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

cdcabrera changed the title ~~WIP, Init Containers~~ [WIP] Init Containers May 19, 2017

spadgett self-requested a review May 20, 2017 18:11

spadgett self-assigned this May 20, 2017

spadgett requested changes May 22, 2017

View reviewed changes

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

cdcabrera force-pushed the init-containers branch 2 times, most recently from f01e52e to 9ce4398 Compare May 22, 2017 19:42

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

benjaminapetersen reviewed May 23, 2017

View reviewed changes

cdcabrera force-pushed the init-containers branch 2 times, most recently from 1181054 to 32f480c Compare May 24, 2017 17:20

cdcabrera force-pushed the init-containers branch 3 times, most recently from b851f05 to c75329e Compare May 25, 2017 15:15

spadgett requested changes May 25, 2017

View reviewed changes

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

cdcabrera force-pushed the init-containers branch from 9d862d0 to 8ca58a0 Compare May 26, 2017 07:46

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

cdcabrera force-pushed the init-containers branch 5 times, most recently from 09da291 to 28d6591 Compare June 1, 2017 15:36

spadgett requested changes Jun 2, 2017

View reviewed changes

cdcabrera force-pushed the init-containers branch from 28d6591 to 920bee8 Compare June 6, 2017 04:57

cdcabrera force-pushed the init-containers branch 3 times, most recently from 4374e04 to 63f12c3 Compare June 9, 2017 18:48

spadgett reviewed Jun 9, 2017

View reviewed changes

cdcabrera force-pushed the init-containers branch 3 times, most recently from ed2655b to 07354d6 Compare June 13, 2017 14:39

spadgett requested changes Jun 13, 2017

View reviewed changes

cdcabrera force-pushed the init-containers branch from 07354d6 to c288842 Compare June 13, 2017 16:01

spadgett reviewed Jun 13, 2017

View reviewed changes

Init Containers Added to Pod Template

fef9211

Update for pod template directive to allow displaying init containers

cdcabrera force-pushed the init-containers branch from c288842 to fef9211 Compare June 13, 2017 20:25

spadgett approved these changes Jun 13, 2017

View reviewed changes

cdcabrera changed the title ~~[WIP] Init Containers~~ Init Containers Jun 13, 2017

openshift-bot merged commit eae97da into openshift:master Jun 14, 2017


		scope.showDebugAction = function (containerStatus) {

		if (_.get(scope.containerStatuses, 'status.phase') === 'Completed') {

Init Containers #1560

Init Containers #1560

Uh oh!

Conversation

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

Uh oh!

Uh oh!

spadgett commented May 19, 2017

Uh oh!

cdcabrera commented May 20, 2017

Uh oh!

spadgett left a comment

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!

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 commented May 24, 2017

Uh oh!

spadgett commented May 24, 2017

Uh oh!

spadgett commented May 24, 2017

Uh oh!

cdcabrera commented May 25, 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!

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!

spadgett left a comment

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!

cdcabrera commented May 19, 2017 •

edited

Loading

cdcabrera commented May 25, 2017 •

edited

Loading

spadgett Jun 2, 2017 •

edited

Loading

cdcabrera commented Jun 9, 2017 •

edited

Loading