Skip to content

On http changes for workflow progress#1

Open
pengz1 wants to merge 3 commits into
masterfrom
on-http-changes-for-workflow-progress
Open

On http changes for workflow progress#1
pengz1 wants to merge 3 commits into
masterfrom
on-http-changes-for-workflow-progress

Conversation

@pengz1

@pengz1 pengz1 commented Oct 13, 2016

Copy link
Copy Markdown
Owner

Code for review.

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
Copy Markdown

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

};

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

Copy link
Copy Markdown

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.

taskName: graphObject[0].tasks[data.taskId].friendlyName,
progress: data.progress
}
};

Copy link
Copy Markdown

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.

return new notificationApiService();
NotificationApiService.prototype.postProgressEvent = function(data) {
var progressData;
return waterline.taskdependencies.find({taskId: data.taskId})

@cgx027 cgx027 Oct 14, 2016

Copy link
Copy Markdown

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.

@cgx027 cgx027 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great overall. 👍 after review comments.

pengz1 pushed a commit that referenced this pull request Jul 6, 2017
add callback handler for wsman microservices
pengz1 pushed a commit that referenced this pull request Aug 18, 2017
Add SEL/LC logging via inventory microservice.  Expose through northb…
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants