Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build hook editor on build configuration page #680

Merged
merged 1 commit into from
Jan 18, 2017
Merged

Build hook editor on build configuration page #680

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions app/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,7 @@ <h1>JavaScript Required</h1>
<script src="bower_components/ace-builds/src-min-noconflict/mode-yaml.js"></script>
<script src="bower_components/ace-builds/src-min-noconflict/theme-dreamweaver.js"></script>
<script src="bower_components/ace-builds/src-min-noconflict/theme-eclipse.js"></script>
<script src="bower_components/ace-builds/src-min-noconflict/mode-sh.js"></script>
<script src="bower_components/angular-ui-ace/ui-ace.js"></script>
<script src="bower_components/clipboard/dist/clipboard.js"></script>
<script src="bower_components/ansi_up/ansi_up.js"></script>
Expand Down
1 change: 1 addition & 0 deletions app/scripts/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": "dev_guide/builds.html#build-hooks",
// default should remain last, add new links above
"default": "welcome/index.html"
},
Expand Down
81 changes: 79 additions & 2 deletions app/scripts/controllers/edit/buildConfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@ angular.module('openshiftConsole')
"title": "Inline"
}];
$scope.view = {
advancedOptions: false
advancedOptions: false,
hasHooks: false
};
$scope.breadcrumbs = [
{
Expand Down Expand Up @@ -117,6 +118,79 @@ angular.module('openshiftConsole')
"SerialLatestOnly"
];

$scope.buildHookTypes = [{
id: "command",
label: "Command"
}, {
id: "script",
label: "Shell Script"
}, {
id: "args",
label: "Arguments to default image entry point"
}, {
id: "commandArgs",
label: "Command with arguments"
}, {
id: "scriptArgs",
label: "Shell script with arguments"
}];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per @jwforres let's make these all sentence case

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


$scope.buildHookSelection = {
type: {}
};

var getInitialBuildHookSelection = function() {
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.script');
$scope.view.hasHooks = hasArgs || hasCommand || hasScript;

var id;

if (hasArgs && hasCommand) {
id = 'commandArgs';
} else if (hasArgs && hasScript) {
id = 'scriptArgs';
} else if (hasArgs) {
id = 'args';
} else if (hasScript) {
id = 'script';
} else {
id = 'command';
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@spadgett spadgett Jan 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 });


$scope.buildHookSelection.type = _.find($scope.buildHookTypes, { id: id });
};

var clearIncompatibleBuildHookFields = function() {
if (!$scope.view.hasHooks) {
delete $scope.updatedBuildConfig.spec.postCommit.command;
delete $scope.updatedBuildConfig.spec.postCommit.args;
delete $scope.updatedBuildConfig.spec.postCommit.script;
} else {
switch ($scope.buildHookSelection.type.id) {
case "script":
delete $scope.updatedBuildConfig.spec.postCommit.command;
delete $scope.updatedBuildConfig.spec.postCommit.args;
break;
case "command":
delete $scope.updatedBuildConfig.spec.postCommit.script;
delete $scope.updatedBuildConfig.spec.postCommit.args;
break;
case "args":
delete $scope.updatedBuildConfig.spec.postCommit.script;
delete $scope.updatedBuildConfig.spec.postCommit.command;
break;
case "scriptArgs":
delete $scope.updatedBuildConfig.spec.postCommit.command;
break;
case "commandArgs":
delete $scope.updatedBuildConfig.spec.postCommit.script;
break;
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

};

AlertMessageService.getAlerts().forEach(function(alert) {
$scope.alerts[alert.name] = alert.data;
});
Expand All @@ -143,6 +217,8 @@ angular.module('openshiftConsole')
// success
function(buildConfig) {
$scope.buildConfig = buildConfig;
getInitialBuildHookSelection();

$scope.updatedBuildConfig = angular.copy($scope.buildConfig);
$scope.buildStrategy = buildStrategy($scope.updatedBuildConfig);
$scope.strategyType = $scope.buildConfig.spec.strategy.type;
Expand Down Expand Up @@ -180,7 +256,7 @@ angular.module('openshiftConsole')
}

if (imageOptions.type === "ImageStreamImage") {
isimage = (imageData.namespace || buildConfig.metadata.namespace) + "/" + imageData.name;
isimage = (imageData.namespace || buildConfig.metadata.namespace) + "/" + imageData.name;
} else {
isimage = "";
}
Expand Down Expand Up @@ -414,6 +490,7 @@ angular.module('openshiftConsole')

$scope.save = function() {
$scope.disableInputs = true;
clearIncompatibleBuildHookFields();
// Update Configuration
buildStrategy($scope.updatedBuildConfig).forcePull = $scope.options.forcePull;

Expand Down
1 change: 1 addition & 0 deletions app/scripts/directives/editCommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ angular.module('openshiftConsole')
restrict: 'E',
scope: {
args: '=',
type: '@',
isRequired: '='
},
templateUrl: 'views/directives/_edit-command.html',
Expand Down
23 changes: 14 additions & 9 deletions app/views/directives/_edit-command.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<ng-form name="form">
<p ng-hide="input.args.length"><em>No command set.</em></p>
<p ng-hide="input.args.length"><em>No {{type || 'command'}} set.</em></p>
<p ng-show="input.args.length" as-sortable ng-model="input.args" class="command-args">
<span ng-repeat="arg in input.args" as-sortable-item class="form-group">
<span class="input-group">
Expand All @@ -25,23 +25,23 @@
<span as-sortable-item-handle class="input-group-addon action-button drag-handle">
<i class="fa fa-bars" aria-hidden="true"></i>
</span>
<a href="" ng-click="removeArg($index)" class="input-group-addon action-button remove-arg" title="Remove Argument">
<span class="sr-only">Remove Argument</span>
<a href="" ng-click="removeArg($index)" class="input-group-addon action-button remove-arg" title="Remove Item">
<span class="sr-only">Remove Item</span>
<i class="pficon pficon-close" aria-hidden="true"></i>
</a>
</span>
</span>
</p>
<div class="form-group">
<label class="sr-only" ng-attr-for="{{id}}-add-arg">Add Argument</label>
<label class="sr-only" ng-attr-for="{{id}}-add-arg">Add {{type || 'Command'}}</label>
<!-- 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'}}"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

class="form-control"
autocorrect="off"
autocapitalize="off"
Expand All @@ -64,7 +64,7 @@
name="nextArg"
rows="10"
ng-attr-id="{{id}}-add-arg"
placeholder="Add argument"
placeholder="Add {{type || 'command'}}"
class="form-control"
autocorrect="off"
autocapitalize="off"
Expand All @@ -81,14 +81,19 @@
</span>
</div>
<div class="help-block">
Enter the command to run inside the container. The command is considered successful if its exit code is 0.
Drag and drop command arguments to reorder them.
<div ng-if="!type">
Enter the command to run inside the container. The command is considered successful if its exit code is 0.
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

</div>
</div>
<div class="mar-top-sm mar-bottom-md">
<a href="" ng-click="multiline = !multiline">Switch to {{multiline ? 'Single-line' : 'Multiline'}} Editor</a>
<span ng-show="input.args.length">
<span class="action-divider">|</span>
<a href="" ng-click="clear()" role="button">Clear Command</a>
<a href="" ng-click="clear()" role="button">Clear {{ (type || 'Command') | sentenceCase }}</a>
</span>
</div>
<!-- Add a hidden input to help with form validation. -->
Expand Down
75 changes: 68 additions & 7 deletions app/views/edit/build-config.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,6 @@ <h3>Source Configuration</h3>
<div ui-ace="{
mode: 'dockerfile',
theme: 'dreamweaver',
onLoad: aceLoaded,
rendererOptions: {
fadeFoldWidgets: true,
showPrintMargin: false
Expand Down Expand Up @@ -275,7 +274,6 @@ <h3 ng-class="{ 'with-divider': !sources.none }">Jenkins Pipeline Configuration<
<div ui-ace="{
mode: 'groovy',
theme: 'eclipse',
onLoad: aceLoaded,
rendererOptions: {
fadeFoldWidgets: true,
showPrintMargin: false
Expand Down Expand Up @@ -416,8 +414,6 @@ <h3 class="with-divider">Image Configuration</h3>
</dl>
</div>



<div ng-if="!(updatedBuildConfig | isJenkinsPipelineStrategy)" class="section">
<h3 class="with-divider">Environment Variables<span class="help action-inline">
<a href="">
Expand Down Expand Up @@ -523,8 +519,8 @@ <h3 class="with-divider">Run Policy
{{type | sentenceCase}}
</ui-select-choices>
</ui-select>

</div>

<div ng-switch="updatedBuildConfig.spec.runPolicy">
<div class="help-block" ng-switch-when="Serial">Builds triggered from this Build Configuration will run one at the time, in the order they have been triggered.</div>
<div class="help-block" ng-switch-when="Parallel">Builds triggered from this Build Configuration will run all at the same time. The order in which they will finish is not guaranteed.</div>
Expand All @@ -533,8 +529,73 @@ <h3 class="with-divider">Run Policy
</div>
</div>

<div>
<a href="" ng-click="view.advancedOptions = !view.advancedOptions" role="button">{{view.advancedOptions ? 'Hide' : 'Show'}} Advanced Options</a>
<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>
</span>
</h3>

<div class="checkbox">
<label>
<input type="checkbox" ng-model="view.hasHooks" id="enable-build-hooks">
Run build hooks after image is built
</label>
<div class="help-block">
Build hooks allow you to run commands at the end of the build to verify the image.
</div>
</div>

<div ng-show="view.hasHooks">
<div class="form-group">
<h4>Hook Types</h4>
<ui-select
ng-model="buildHookSelection.type"
title="Choose a type of build hook">
<ui-select-match>{{$select.selected.label}}</ui-select-match>
<ui-select-choices repeat="type in buildHookTypes">
{{type.label}}
</ui-select-choices>
</ui-select>
</div>

<fieldset>
<div ng-show="buildHookSelection.type.id === 'script' || buildHookSelection.type.id === 'scriptArgs'">
<h4>Script</h4>
<div
ui-ace="{
mode: 'sh',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

theme: 'eclipse',
rendererOptions: {
fadeFoldWidgets: true,
showPrintMargin: false
}
}"
ng-model="updatedBuildConfig.spec.postCommit.script"
class="ace-bordered ace-inline mar-top-md">
</div>
</div>

<div ng-show="buildHookSelection.type.id === 'command' || buildHookSelection.type.id === 'commandArgs'">
<h4>Command</h4>
<edit-command
args="updatedBuildConfig.spec.postCommit.command">
</edit-command>
</div>

<div ng-show="buildHookSelection.type.id === 'args' || buildHookSelection.type.id === 'commandArgs' || buildHookSelection.type.id === 'scriptArgs' ">
<h4>Arguments</h4>
<edit-command
args="updatedBuildConfig.spec.postCommit.args"
type="argument">
</edit-command>
</div>
</fieldset>
</div>
</div>

<div class="gutter-top">
<a href="" ng-click="view.advancedOptions = !view.advancedOptions" role="button">{{view.advancedOptions ? 'Hide' : 'Show'}} advanced options</a>
</div>
<div class="buttons gutter-top-bottom">
<button
Expand Down
3 changes: 2 additions & 1 deletion bower.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,8 @@
"src-min-noconflict/mode-groovy.js",
"src-min-noconflict/mode-yaml.js",
"src-min-noconflict/theme-dreamweaver.js",
"src-min-noconflict/theme-eclipse.js"
"src-min-noconflict/theme-eclipse.js",
"src-min-noconflict/mode-sh.js"
]
}
}
Expand Down
Loading