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

On http changes for workflow progress #1

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

On http changes for workflow progress #1

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4e18b96
POC-for-workflow progress
pengz1 Oct 13, 2016
e264abb
Ingore notification API when there is no active task
pengz1 Oct 13, 2016
0a4c2f2
Modify secure_erase.py for progress parser
pengz1 Oct 17, 2016
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
71 changes: 53 additions & 18 deletions lib/services/notification-api-service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,14 @@

var di = require('di');

module.exports = notificationApiServiceFactory;
di.annotate(notificationApiServiceFactory, new di.Provide('Http.Services.Api.Notification'));
di.annotate(notificationApiServiceFactory,
module.exports = NotificationApiServiceFactory;
di.annotate(NotificationApiServiceFactory, new di.Provide('Http.Services.Api.Notification'));
di.annotate(NotificationApiServiceFactory,
new di.Inject(
'Protocol.Events',
'Protocol.Task',
'TaskGraph.Store',
'TaskGraph.TaskGraph',
'Logger',
'Services.Waterline',
'Errors',
Expand All @@ -19,45 +20,46 @@ di.annotate(notificationApiServiceFactory,
)
);

function notificationApiServiceFactory(
function NotificationApiServiceFactory(
eventsProtocol,
taskProtocol,
taskGraphStore,
TaskGraph,
Logger,
waterline,
Errors,
Promise,
_
) {
var logger = Logger.initialize(notificationApiServiceFactory);
var logger = Logger.initialize(NotificationApiServiceFactory);

function notificationApiService() {
function NotificationApiService() {
}

notificationApiService.prototype.postNotification = function(message) {
NotificationApiService.prototype.postNotification = function(message) {
var self = this;

if (_.has(message, 'nodeId')) {
return self.postNodeNotification(message);
} else if (_.has(message, 'taskId') && _.has(message, 'progress')) {
// This will be progress update notification if taskId is specified
return self.postProgressEvent(message);
Copy link

Choose a reason for hiding this comment

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

I would suggest to refactor this to following so other develops can choose to develop other task notifications other than progress notification.

       } else if (_.has(message, 'taskId')) {
            if (_.has(message, 'progress'))
            {
                // This will be progress update notification if taskId is specified
                return self.postProgressEvent(message);
            }
            if (_.has(message, 'SOMETHING_ELSE'))
            {
                // others can choose to post other kind of task notification without need to modify your code. 
            }

}
// Add other cases here if to support more notification types

// This will be a broadcast notification if no id (like nodeId) is specified
else {
// This will be a broadcast notification if no id (like nodeId) is specified
return self.postBroadcastNotification(message);
}
};

notificationApiService.prototype.postNodeNotification = function(message) {
var self = this;
NotificationApiService.prototype.postNodeNotification = function(message) {

return Promise.try(function() {
if (!message.nodeId || !_.isString(message.nodeId)) {
throw new Errors.BadRequestError('Invalid node ID in query or body');
};
}
})
.then(function () {
return waterline.nodes.needByIdentifier(message.nodeId)
return waterline.nodes.needByIdentifier(message.nodeId);
})
.then(function (node) {
if(!node) {
Expand All @@ -70,14 +72,47 @@ function notificationApiServiceFactory(
});
};

notificationApiService.prototype.postBroadcastNotification = function(message) {
var self = this;

NotificationApiService.prototype.postBroadcastNotification = function(message) {
return eventsProtocol.publishBroadcastNotification(message)
.then(function () {
return message;
});
};

return new notificationApiService();
NotificationApiService.prototype.postProgressEvent = function(data) {
Copy link

Choose a reason for hiding this comment

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

I would suggest to change function name to postProgressNotification to be better aligned with previous function. This is not a big deal anyway.

var progressData;
return waterline.taskdependencies.find({taskId: data.taskId})
Copy link

@cgx027 cgx027 Oct 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Copy @yyscamper 's comments at on-taskgraph:

use 'findOne instead offind, so that you can use graphObject rather
than graphObject[0]` in following code.

.then(function(task){
if (!_.isEmpty(task) && _.has(task[0], 'graphId')){
return waterline.graphobjects.find({instanceId: task[0].graphId})
.then(function(graphObject){
//TODO: workflow progress percentage should be designed
progressData = {
graphId: graphObject[0].instanceId,
graphName: graphObject[0].definition.friendlyName,
progress: {
percentage: "na",
description: data.progress.description
},
taskProgress: {
graphId: graphObject[0].instanceId,
taskId: data.taskId,
taskName: graphObject[0].tasks[data.taskId].friendlyName,
progress: data.progress
}
};
Copy link

Choose a reason for hiding this comment

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

suggest to return something in this .then block. One option would be combine following then block with this one.

})
.then(function(){
return TaskGraph.prototype.updateGraphProgress(progressData);
});
} else {
logger.error('notification API fails', {
progress: data,
error: "Can't find active task for given taskId"
});
}
});
};

return new NotificationApiService();
}
11 changes: 11 additions & 0 deletions lib/services/workflow-api-service.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,17 @@ function workflowApiServiceFactory(
definition, configuration.options, context, configuration.domain, true);
});
})
.tap(function(graph){
var progressData = {
graphId: graph.instanceId,
progress: {
percentage: "0%",
description: 'Graph "' + graph.definition.friendlyName + '" started'
},
taskProgress: {}
};
return TaskGraph.prototype.updateGraphProgress(progressData);
})
.then(function(graph) {
self.runTaskGraph(graph.instanceId, configuration.domain);
return graph;
Expand Down
82 changes: 79 additions & 3 deletions spec/lib/services/notification-api-service-spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ describe('Http.Api.Notification', function () {
var needByIdentifier;
var postNodeNotification;
var postBroadcastNotification;
var TaskGraph;

var nodeNotificationMessage = {
nodeId: "57a86b5c36ec578876878294",
Expand All @@ -20,22 +21,32 @@ describe('Http.Api.Notification', function () {
data: 'dummy data'
};

var node = {_id: nodeNotificationMessage.nodeId}
var progressNotificationMessage = {
taskId: "57a86b5c36ec578876878294",
progress: {
description: "test",
percentage: "10%"
}
};

var node = {_id: nodeNotificationMessage.nodeId};

before('Setup mocks', function () {
helper.setupInjector([
helper.require("/lib/services/notification-api-service.js")
onHttpContext.prerequisiteInjectables,
helper.require("/lib/services/notification-api-service.js"),
]);
notificationApiService = helper.injector.get('Http.Services.Api.Notification');
_ = helper.injector.get('_');
eventProtocol = helper.injector.get('Protocol.Events');
waterline = helper.injector.get('Services.Waterline');
TaskGraph = helper.injector.get('TaskGraph.TaskGraph');
waterline.nodes = {
needByIdentifier: function() {}
};
sinon.stub(eventProtocol, 'publishNodeNotification').resolves();
sinon.stub(eventProtocol, 'publishBroadcastNotification').resolves();

this.sandbox = sinon.sandbox.create();
needByIdentifier = sinon.stub(waterline.nodes, 'needByIdentifier');
needByIdentifier.resolves(node);
postNodeNotification = sinon.spy(notificationApiService, 'postNodeNotification');
Expand Down Expand Up @@ -112,5 +123,70 @@ describe('Http.Api.Notification', function () {
expect(resp).to.deep.equal(broadcastNotificationMessage);
});
});

it('should update graph progress', function () {
var tasks = [{graphId: "graphId"}],
graphs = [{
instanceId: "graphId",
definition: {friendlyName: "Test Graph"},
tasks: {"57a86b5c36ec578876878294": {friendlyName: "Test Task"}}
}],
progressData = {
graphId: graphs[0].instanceId,
graphName: graphs[0].definition.friendlyName,
progress: {
percentage: "na",
description: progressNotificationMessage.progress.description
},
taskProgress: {
graphId: graphs[0].instanceId,
taskId: progressNotificationMessage.taskId,
taskName: graphs[0].tasks[progressNotificationMessage.taskId].friendlyName,
progress: progressNotificationMessage.progress
}
};
waterline.taskdependencies = {find: function() {}};
waterline.graphobjects = {find: function() {}};
this.sandbox.stub(waterline.taskdependencies, 'find').resolves(tasks);
this.sandbox.stub(waterline.graphobjects, 'find').resolves(graphs);
this.sandbox.stub(TaskGraph.prototype, 'updateGraphProgress').resolves();
return notificationApiService.postProgressEvent(progressNotificationMessage)
.then(function () {
expect(waterline.taskdependencies.find).to.be.calledOnce;
expect(waterline.taskdependencies.find).to.be.calledWith({
taskId: progressNotificationMessage.taskId});
expect(waterline.graphobjects.find).to.be.calledOnce;
expect(waterline.graphobjects.find).to.be.calledWith({
instanceId: tasks[0].graphId});
expect(TaskGraph.prototype.updateGraphProgress).to.be.calledOnce;
expect(TaskGraph.prototype.updateGraphProgress).to.be.calledWith(progressData);
});
});

it('should not update graph progress', function () {
this.sandbox.restore();
waterline.taskdependencies = {find: function() {}};
waterline.graphobjects = {find: function() {}};
this.sandbox.stub(waterline.taskdependencies, 'find').resolves([]);
this.sandbox.spy(waterline.graphobjects, 'find');
this.sandbox.spy(TaskGraph.prototype, 'updateGraphProgress');
return notificationApiService.postProgressEvent({taskId: 'aTask'})
.then(function () {
expect(waterline.taskdependencies.find).to.be.calledOnce;
expect(waterline.graphobjects.find).to.have.not.been.called;
expect(TaskGraph.prototype.updateGraphProgress).to.have.not.been.called;
});
});

it('should call postProgressEvent', function () {
sinon.stub(notificationApiService, 'postProgressEvent').resolves();
return notificationApiService.postNotification(progressNotificationMessage)
.then(function () {
expect(notificationApiService.postProgressEvent).to.be.calledOnce;
expect(notificationApiService.postProgressEvent).to.be
.calledWith(progressNotificationMessage);
});
});

});
});
17 changes: 15 additions & 2 deletions spec/lib/services/workflow-api-service-spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,10 @@ describe('Http.Services.Api.Workflows', function () {
waterline.taskdefinitions = {
destroy: sinon.stub().resolves({ injectableName: 'test' })
};
graph = { instanceId: 'testgraphid' };
graph = {
instanceId: 'testgraphid',
definition: {friendlyName: 'testGraph'}
};
task = { instanceId: 'testtaskid' };
workflow = { id: 'testid', _status: 'cancelled' };
graphDefinition = { injectableName: 'Graph.Test' };
Expand All @@ -73,6 +76,7 @@ describe('Http.Services.Api.Workflows', function () {
this.sandbox.stub(workflowApiService, 'createActiveGraph');
this.sandbox.stub(workflowApiService, 'runTaskGraph');
this.sandbox.stub(env, 'get');
this.sandbox.stub(TaskGraph.prototype, 'updateGraphProgress').resolves();
});

afterEach('Http.Services.Api.Profiles afterEach', function() {
Expand All @@ -87,7 +91,14 @@ describe('Http.Services.Api.Workflows', function () {
workflowApiService.findGraphDefinitionByName.resolves(graphDefinition);
workflowApiService.createActiveGraph.resolves(graph);
workflowApiService.runTaskGraph.resolves();

var data = {
graphId: graph.instanceId,
progress: {
percentage: "0%",
description: 'Graph "' + graph.definition.friendlyName + '" started'
},
taskProgress: {}
};
return workflowApiService.createAndRunGraph({
name: 'Graph.Test',
options: { test: 1 },
Expand All @@ -103,6 +114,8 @@ describe('Http.Services.Api.Workflows', function () {
expect(workflowApiService.createActiveGraph).to.have.been.calledWith(
graphDefinition, { test: 1 }, { test: 2 }, 'test'
);
expect(TaskGraph.prototype.updateGraphProgress).to.have.been.calledOnce;
expect(TaskGraph.prototype.updateGraphProgress).to.have.been.calledWith(data);
expect(workflowApiService.runTaskGraph).to.have.been.calledOnce;
expect(workflowApiService.runTaskGraph)
.to.have.been.calledWith(graph.instanceId, 'test');
Expand Down