Build hook editor on build configuration page #680

MarkDeMaria · 2016-10-14T20:42:52Z

-Multi-select tag for Command, Script, and Args
-Warning states:
--On page load, if both Command and Script are both present, forms are disabled and user told to use YAML
--If either Command or Script present, and user tries to add the type not present, Save button is disabled and has-error info-block informs user of error and told to use YAML

All Selected

Script & Command present at page load. Form disabled and notification present

Page loaded with either script or command. Entered the other. Save button disabled and message shown to user.

spadgett · 2016-10-17T14:42:13Z

@MarkDeMaria I spoke to @jwforres, and we decided that it's better to have a ui-select with only the valid options:

Shell Script
Command
Arguments to Default Image Entry Point
Shell Script with Arguments
Command with Arguments

This prevents users from selecting an invalid combination.

MarkDeMaria · 2016-10-27T20:27:04Z

As discussed, now uses UI-Select with each possible combination. Selecting save will overwrite the postCommit data with what is being shown on the page. For example, if saving on the Script+Args page while Commands has data, Commands will be wiped in favor of Script+Args, but Commands will still be accessible prior to saving if user switches back to the Commands drop-down.

Added bonus: Page will auto-select a dropdown option based on what is saved in Post Commit.

One problem that needs addressing is that the edit-command directive needs to be able to replace the phrase "No Command set" with "No Argument set." See screenshots below.

Drop Down

Script + Args

Command + Args

spadgett · 2016-10-28T21:01:22Z

Can you add "Learn more" link to this topic?

https://docs.openshift.org/latest/dev_guide/builds.html#build-hooks

spadgett · 2016-10-28T20:40:09Z

app/scripts/controllers/edit/buildConfig.js

+      "Arguments to Default Image Entry Point",
+      "Shell Script with Arguments",
+      "Command with Arguments"
+    ];


I'd prefer if we had a label and internal ID for each of these. I'd rather not do string comparisons on long labels, and it will make translation harder down the road.

spadgett · 2016-10-28T20:43:41Z

app/scripts/controllers/edit/buildConfig.js

+        $scope.buildHookSelection.type = "Command";
+      } else {
+        $scope.buildHookSelection.type = "Shell Script";
+      }


I think something like this would be easier to follow:

var postCommit = _.get($scope, 'buildConfig.spec.postCommit', {}); if (_.size(postCommit.args) && _.size(postCommit.command)) { $scope.buildHookSelection.type = "Command with Arguments"; return; } if (postCommit.script && _.size(postCommit.args)) { $scope.buildHookSelection.type = "Shell Script with Arguments"; return; } // etc...

Do we need an option for no post commit hooks? How do I unset the value?

spadgett · 2016-10-28T20:44:01Z

app/scripts/controllers/edit/buildConfig.js

+    var getInitialBuildHookSelection = function() {
+      if (($scope.buildConfig.spec.postCommit.args || []).length) {
+        if (($scope.buildConfig.spec.postCommit.command || []).length) {
+          $scope.buildHookSelection.type = "Command with Arguments";sdas


Random trailing chars

spadgett · 2016-10-28T20:54:40Z

app/views/edit/build-config.html

+                        <h3 class="with-divider">Build Hooks
+                          <span class="help action-inline">
+                            <a href>
+                              <i class="pficon pficon-help" data-toggle="tooltip" aria-hidden="true" data-original-title="Build hooks allow behavior to be injected into the build process."></i>


"Build hooks let you run commands at the end of the build to verify the image."

spadgett · 2016-10-28T20:55:34Z

app/views/edit/build-config.html

+                            <h4>Hook Types</h4>
+                            <ui-select required
+                                       ng-model="buildHookSelection.type"
+                                       title="Choose a Build Hook type to edit">


Choose a type of build hook

spadgett · 2016-10-28T21:02:51Z

app/views/edit/build-config.html

+                              <h4>Command</h4>
+                              <edit-command
+                                args="buildConfig.spec.postCommit.command"
+                                isRequired="true">


I'm not sure setting isRequired="true" is correct since this is inside an ng-show block, not an ng-if. If I don't have a command and switch types, can I submit the form?

spadgett · 2016-10-28T21:03:33Z

app/views/edit/build-config.html

+                                ui-ace="{
+                                  mode: 'yaml',
+                                  theme: 'eclipse',
+                                  onLoad: aceLoaded,


aceLoaded is not defined in the controller, remove onLoad

Looks like Line 112 & Line 293 both have onLoad: aceLoaded as well. Want me remove them too?

Yes, it shouldn't be there unless there is a controller aceLoaded method. (I don't think there is.)

spadgett · 2016-10-28T21:03:40Z

app/views/edit/build-config.html

+                                ui-ace="{
+                                  mode: 'yaml',
+                                  theme: 'eclipse',
+                                  onLoad: aceLoaded,


aceLoaded is not defined in the controller, remove onLoad

spadgett · 2016-10-28T21:05:24Z

app/views/edit/build-config.html

+                                    showPrintMargin: false
+                                  }
+                                }"
+                                ng-model="buildConfig.spec.postCommit.script"


All of these fields should use updatedBuildConfig in the model. You shouldn't mutate the original build config.

spadgett · 2016-10-28T21:06:54Z

app/views/edit/build-config.html

-
+
+                    <div class="section" ng-if="view.advancedOptions">
+                      <div ng-if="!(updatedBuildConfig | isJenkinsPipelineStrategy)" class="section">


Maybe make this div and its parent one div

<div ng-if="view.advancedOptions && !(updatedBuildConfig | isJenkinsPipelineStrategy)" class="section">

MarkDeMaria · 2016-12-16T19:29:56Z

@spadgett @jwforres

Build Hook Checkbox unselected

Build Hook Checkbox selected

jwforres · 2016-12-16T21:32:35Z

i think sentence case would look / read better in the dropdown "Shell script with arguments"

…

On Fri, Dec 16, 2016 at 2:29 PM, Mark DeMaria ***@***.***> wrote: @spadgett <https://github.com/spadgett> @jwforres <https://github.com/jwforres> Build Hook Checkbox unselected [image: image] <https://cloud.githubusercontent.com/assets/19572090/21275667/e99ed41a-c39b-11e6-8c4c-7dfc9f227d5f.png> Build Hook Checkbox selected [image: image] <https://cloud.githubusercontent.com/assets/19572090/21275699/19928586-c39c-11e6-9166-62da9cea31a7.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#680 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABZk7YYp_4d5APiwcERdV4UEgM6kcQ03ks5rIua1gaJpZM4KXa-k> .

MarkDeMaria · 2016-12-16T21:53:59Z

Can do. Do you want sentence-case for all drop-down entries?

spadgett · 2017-01-11T18:20:19Z

app/scripts/constants.js

@@ -51,6 +51,7 @@ window.OPENSHIFT_CONSTANTS = {
    "builds":                  "architecture/core_concepts/builds_and_image_streams.html#builds",
    "image-streams":           "architecture/core_concepts/builds_and_image_streams.html#image-streams",
    "storage":                 "architecture/additional_concepts/storage.html",
+    "build-hooks":             "https://docs.openshift.org/latest/dev_guide/builds.html#build-hooks",


Make it a relative URL.

"dev_guide/builds.html#build-hooks"

spadgett · 2017-01-11T18:21:03Z

app/scripts/controllers/edit/buildConfig.js

+    }, {
+      "id": "commandArgs",
+      "label": "Command with Arguments"
+    }];


Per @jwforres let's make these all sentence case

Nit: the quotes around id and label aren't needed.

spadgett · 2017-01-11T18:21:41Z

app/scripts/controllers/edit/buildConfig.js

+    }];
+
+    $scope.buildHookSelection = {
+      "type": {}


The quotes are type aren't needed.

spadgett · 2017-01-11T18:30:21Z

app/scripts/controllers/edit/buildConfig.js

+    var getInitialBuildHookSelection = function() {
+      $scope.view.hasHooks = (_.size($scope.buildConfig.spec.postCommit.args) ||
+                              _.size($scope.buildConfig.spec.postCommit.command) ||
+                              $scope.buildConfig.spec.postCommit.script) ? true : false;


I don't think the postCommit property is guaranteed to be in the JSON. Using _.get guards against a runtime error.

var hasArgs = !_.isEmpty(_.get($scope, 'buildConfig.spec.postCommit.args')); var hasCommand = !_.isEmpty(_.get($scope, 'buildConfig.spec.postCommit.command')); var hasScript = !!_.get($scope, 'buildConfig.spec.postCommit.command'); $scope.view.hasHooks = hasArgs || hasCommand || hasScript;

Then you can use the local vars below without checking size again.

spadgett · 2017-01-11T18:32:46Z

app/scripts/controllers/edit/buildConfig.js

+        $scope.buildHookSelection.type.id = "command";
+      }
+
+      $scope.buildHookSelection.type.label = _.find($scope.buildHookTypes, {'id': $scope.buildHookSelection.type.id}).label;


Shouldn't need to set this. You can use {{$select.selected.label}} in the ui-select markup.

spadgett · 2017-01-11T18:38:42Z

app/scripts/controllers/edit/buildConfig.js

+      $scope.buildHookSelection.type.label = _.find($scope.buildHookTypes, {'id': $scope.buildHookSelection.type.id}).label;
+    };
+
+    var clearIncompatibleBuildHookFields = function(selectedBuildHookID) {


I'd delete the incompatible fields instead of setting them to empty values.

spadgett · 2017-01-11T18:40:00Z

app/scripts/controllers/edit/buildConfig.js

@@ -414,6 +487,7 @@ angular.module('openshiftConsole')

    $scope.save = function() {
      $scope.disableInputs = true;
+      clearIncompatibleBuildHookFields($scope.buildHookSelection.type.id);


I'd check $scope.buildHookSelection.type.id directly in the clearIncompatibleBuildHookFields instead of passing it in.

spadgett · 2017-01-11T18:41:41Z

app/views/edit/build-config.html

+                    <div ng-if="view.advancedOptions && !(updatedBuildConfig | isJenkinsPipelineStrategy)" class="section">
+                      <h3 class="with-divider">Build Hooks
+                        <span class="help action-inline">
+                          <a href="{{'build-hooks' | helpLink}}" aria-hidden="true" target="_blank"><span class="learn-more-inline">Learn more&nbsp;<i class="fa fa-external-link"></i></span></a>


We've changed the "Learn More" links to use an uppercase M.

spadgett · 2017-01-11T18:44:11Z

app/views/edit/build-config.html

+                            <h4>Script</h4>
+                            <div
+                              ui-ace="{
+                                mode: 'sh',


Need to add mode-sh.js to ace-builds in bower.json.

spadgett

See comments

spadgett · 2017-01-11T18:54:21Z

Need to update edit-command to customize the "No command" message and help text for this context.

MarkDeMaria · 2017-01-13T19:54:52Z

@spadgett @jwforres All changes made with the exception of the edit-command directive alterations. Do you want that as a separate PR? And do you have any other categories (Command, Arg, Script, etc) you'd like supported for that directive?

spadgett · 2017-01-13T20:19:04Z

I think it's small enough to add this PR. I'd make the label an optional attribute I can pass into the directive.

spadgett

You have a change in dist.java that shouldn't be there. We'll have to fix that before we merge.

Note this needs to be in by the end of tomorrow (Wed Jan 18) to make feature cutoff for 1.5. We can make the edit-command directive changes a follow-on if necessary.

Thanks @MarkDeMaria

spadgett · 2017-01-17T17:07:03Z

app/scripts/controllers/edit/buildConfig.js

+        $scope.buildHookSelection.type = _.find($scope.buildHookTypes, { id: 'script' });
+      } else {
+        $scope.buildHookSelection.type = _.find($scope.buildHookTypes, { id: 'command' });
+      }


Nit: I'd assign the id to a var and the only do the _.find in one place. That avoids repeating the same code.

var id; if (hasArgs && hasCommand) { id = 'commandArgs'; } else if ... $scope.buildHookSelection.type = _.find($scope.buildHookTypes, { id: id });

spadgett · 2017-01-17T17:18:22Z

app/scripts/controllers/edit/buildConfig.js

+        $scope.updatedBuildConfig.spec.postCommit.command = [];
+        $scope.updatedBuildConfig.spec.postCommit.args = [];
+        $scope.updatedBuildConfig.spec.postCommit.script = "";
+      }


What you have here is fine, although I might handle the simple case first and return early. It makes the code a little easier to understand because it avoids a big if block.

var clearIncompatibleBuildHookFields = function() { if (!$scope.view.hasHooks) { $scope.updatedBuildConfig.spec.postCommit.command = []; $scope.updatedBuildConfig.spec.postCommit.args = []; $scope.updatedBuildConfig.spec.postCommit.script = ""; return; } switch ($scope.buildHookSelection.type.id) { // ... } };

Probably should delete the keys for !hasHooks case as well instead of setting them to empty values.

-Single-select tag for Command, Script, and Args, Command+Args, Script+Args -Whatever option is currently-selected upon hitting 'Save' will overwrite anything else. For example, if post commit has a command in it, and you hit save on the Args page, the command will be deleted and the args saved. -Checkbox shows/hides the build hook editor. Leaving it unchcked and selecting "Save" with anything saved in the post commit section of the YAML will be deleted, as the user has indicated they do not wish to have any build hooks -Alterations to edit-command directive to allow for a 'type' arg to be passed through. Will replace relevant wording. For example, 'argument.' If no type provided, defaults to 'command.'

MarkDeMaria · 2017-01-18T16:07:13Z

@spadgett @jwforres Changes to edit-command directive made and bad dist file removed.

spadgett · 2017-01-18T20:03:51Z

app/views/directives/_edit-command.html

    <!-- Single-line entry -->
    <span ng-show="!multiline" class="input-group">
      <input type="text"
             ng-model="nextArg"
             name="nextArg"
             ng-attr-id="{{id}}-add-arg"
             on-enter="addArg()"
-             placeholder="Add argument"
+             placeholder="Add {{type || 'command'}}"


Changing the default will affect other places where this is used, where it should be "Add argument"

spadgett · 2017-01-18T20:07:05Z

app/views/directives/_edit-command.html

+      Drag and drop command arguments to reorder them.
+    </div>
+    <div ng-if="type">
+      Enter the arguments that will be appended to the container's command. Drag and drop arguments to reorder them.


This text is not necessarily always correct when the type is set. We should add a description scope value if we need to say something specific like this.

spadgett · 2017-01-18T21:24:37Z

I'm going to go ahead and merge so that we make DCUT. The remaining comments are fairly minor, and we can fix them in a follow up PR. @jwforres FYI

[merge]

openshift-bot · 2017-01-18T21:31:22Z

Evaluated for origin web console merge up to a968944

openshift-bot · 2017-01-18T21:31:30Z

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

spadgett self-assigned this Oct 18, 2016

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

MarkDeMaria force-pushed the postCommitEditor branch from 5446f9c to d9e3dd6 Compare October 27, 2016 18:05

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

MarkDeMaria force-pushed the postCommitEditor branch from d9e3dd6 to 72362c3 Compare October 27, 2016 20:22

MarkDeMaria force-pushed the postCommitEditor branch 3 times, most recently from ed07a16 to 269c025 Compare October 28, 2016 19:48

spadgett requested changes Oct 28, 2016

View reviewed changes

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

MarkDeMaria force-pushed the postCommitEditor branch from 71b96b9 to 675b326 Compare December 16, 2016 19:27

MarkDeMaria force-pushed the postCommitEditor branch from 675b326 to 60bd1bd Compare December 16, 2016 19:56

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

MarkDeMaria force-pushed the postCommitEditor branch from 60bd1bd to 8a3f330 Compare December 16, 2016 21:16

spadgett reviewed Jan 11, 2017

View reviewed changes

spadgett requested changes Jan 11, 2017

View reviewed changes

MarkDeMaria force-pushed the postCommitEditor branch 6 times, most recently from e0407ed to 231ccb6 Compare January 13, 2017 19:48

spadgett reviewed Jan 17, 2017

View reviewed changes

MarkDeMaria force-pushed the postCommitEditor branch from 231ccb6 to b2fa06b Compare January 18, 2017 16:03

MarkDeMaria force-pushed the postCommitEditor branch from b2fa06b to a968944 Compare January 18, 2017 16:07

spadgett requested changes Jan 18, 2017

View reviewed changes

openshift-bot merged commit 04dcf30 into openshift:master Jan 18, 2017

This was referenced Jan 19, 2017

Show post commit hooks for builds and build configs #1139

Merged

Improve placeholder and description for build hook commands #1181

Merged



		<div class="section" ng-if="view.advancedOptions">
		<div ng-if="!(updatedBuildConfig \| isJenkinsPipelineStrategy)" class="section">

Build hook editor on build configuration page #680

Build hook editor on build configuration page #680

Conversation

MarkDeMaria commented Oct 14, 2016

spadgett commented Oct 17, 2016 • edited Loading

MarkDeMaria commented Oct 27, 2016 • edited Loading

spadgett commented Oct 28, 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

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

MarkDeMaria commented Dec 16, 2016

jwforres commented Dec 16, 2016 via email

MarkDeMaria commented Dec 16, 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

spadgett Jan 11, 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

spadgett left a comment

Choose a reason for hiding this comment

spadgett commented Jan 11, 2017

MarkDeMaria commented Jan 13, 2017

spadgett commented Jan 13, 2017

spadgett left a comment

Choose a reason for hiding this comment

spadgett Jan 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkDeMaria commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett commented Jan 18, 2017

openshift-bot commented Jan 18, 2017

openshift-bot commented Jan 18, 2017 • edited Loading

spadgett commented Oct 17, 2016 •

edited

Loading

MarkDeMaria commented Oct 27, 2016 •

edited

Loading

spadgett Jan 11, 2017 •

edited

Loading

spadgett Jan 17, 2017 •

edited

Loading

openshift-bot commented Jan 18, 2017 •

edited

Loading