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.

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

Wsman firmware update task refine #538

Merged
merged 4 commits into from
Nov 8, 2017
Merged

Conversation

nortonluo
Copy link
Contributor

@nortonluo nortonluo commented Oct 31, 2017

Background

The original WSMAN based job is out of date after SMI service updated its input parameters. And also it is lack of unit test and option schema check. Currently we refine all WSMAN implementation and aimed to integrate SMI service with RackHD and replace the RACADM. We refined the WSMAN work job arch. Now it use the new common WSMAN base job as base classs and WSMAN tool as util to send out request to SMI service.

Detail

Refined the job arch to use WSMAN base job as base.
Refined payload send to SMI service.
Refined input parameter to task.
Add task schema for task parameter.
Add unit test for WSMAN control job.
Add unit test for task schema.

Test Result

Tested pass on Dell R730 server.
wsman-control.js Unit test coverage: 97%

Review: @yaolingling @anhou @lanchongyizu @AlaricChan

tgelter and others added 3 commits September 12, 2017 17:39
Refine emc diag fimrware update file upload method

Change HTTP code

Fix unit test bug

Fix unit test coverage

Fix indent

Fix conflict
{status: "Success"},
{status: "Success"}
];
job.firmwareUpdateCallback(callBackData)

Choose a reason for hiding this comment

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

Missing semicolon.

"body": {
"response": "Request Submitted!"
}
}

Choose a reason for hiding this comment

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

Missing semicolon.

shareName: "emc",
sharePassworrd: "usr",
shareType: 2,
}

Choose a reason for hiding this comment

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

Missing semicolon.

if (!data){
throw new errors.NotFoundError('Dell FirmwareUpdate web service for '+ name +' is not defined in wsmanConfig.json.'); //jshint ignore:line
}
};

Choose a reason for hiding this comment

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

Unnecessary semicolon.

if (isSuccessful){
self._done();
}
else {
console.log("gg")

Choose a reason for hiding this comment

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

Missing semicolon.

}
}
});
}

};

Choose a reason for hiding this comment

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

Unnecessary semicolon.

};
logger.info('Before calling the endpoint :');
return self.clientRequest(apiHost,path,method,request);
return self.clientRequest(apiHost, path, method, request)

Choose a reason for hiding this comment

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

Missing semicolon.


if (options.shareType === "cifs" || options.shareType === "CIFS") {
this.shareConfig.shareType = 2;
};

Choose a reason for hiding this comment

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

Unnecessary semicolon.

{status: "Success"},
{status: "Success"}
];
job.firmwareUpdateCallback(callBackData)

Choose a reason for hiding this comment

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

Missing semicolon.

"body": {
"response": "Request Submitted!"
}
}

Choose a reason for hiding this comment

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

Missing semicolon.

shareName: "emc",
sharePassworrd: "usr",
shareType: 2,
}

Choose a reason for hiding this comment

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

Missing semicolon.

if (!data){
throw new errors.NotFoundError('Dell FirmwareUpdate web service for '+ name +' is not defined in wsmanConfig.json.'); //jshint ignore:line
}
};

Choose a reason for hiding this comment

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

Unnecessary semicolon.

if (isSuccessful){
self._done();
}
else {
console.log("gg")

Choose a reason for hiding this comment

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

Missing semicolon.

}
}
});
}

};

Choose a reason for hiding this comment

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

Unnecessary semicolon.

};
logger.info('Before calling the endpoint :');
return self.clientRequest(apiHost,path,method,request);
return self.clientRequest(apiHost, path, method, request)

Choose a reason for hiding this comment

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

Missing semicolon.


if (options.shareType === "cifs" || options.shareType === "CIFS") {
this.shareConfig.shareType = 2;
};

Choose a reason for hiding this comment

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

Unnecessary semicolon.

});

beforeEach(function(){
job = new WsmanJob({}, {"nodeId": nodeId}, uuid.v4());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New job in before


it('Should get error: data is Failed', function(){
var callBackData = [{status: "Failed"}];
expect(function(){
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add fail+success in date

@lanchongyizu
Copy link
Member

Please add run-test label to trigger Concourse CI.

},
node: nodeId
};
var shareConfig = {

Choose a reason for hiding this comment

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

'shareConfig' is defined but never used.

var failureFound = false;
if (data !== null && data.length > 0) {
data.forEach(function (item) {
if ( !(item.status === 'undefined' || item.status === null || item.status === ""|| failureFound == true)){

Choose a reason for hiding this comment

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

Expected '===' and instead saw '=='.

password :"",
};
this.rebootNeeded = options.rebootNeeded || "YES";
this.dellConfigs = undefined;

Choose a reason for hiding this comment

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

If a strict mode function is executed using function invocation, its 'this' value will be undefined.

user :"",
password :"",
};
this.rebootNeeded = options.rebootNeeded || "YES";

Choose a reason for hiding this comment

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

If a strict mode function is executed using function invocation, its 'this' value will be undefined.

if (options.shareFolderType === "cifs" || options.shareFolderType === "CIFS") {
this.shareConfig.shareType = 2;
}
this.targetConfig = {

Choose a reason for hiding this comment

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

If a strict mode function is executed using function invocation, its 'this' value will be undefined.

this.shareConfig.shareType = 0;
}
if (options.shareFolderType === "cifs" || options.shareFolderType === "CIFS") {
this.shareConfig.shareType = 2;

Choose a reason for hiding this comment

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

If a strict mode function is executed using function invocation, its 'this' value will be undefined.

shareType: 2
};
if (options.shareFolderType === "nfs" || options.shareFolderType === "NFS") {
this.shareConfig.shareType = 0;

Choose a reason for hiding this comment

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

If a strict mode function is executed using function invocation, its 'this' value will be undefined.

wsmanFirmwareUpdateJob.super_.call(this, logger, options, context, taskId);

this.nodeId = this.context.target;
this.shareConfig = {

Choose a reason for hiding this comment

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

If a strict mode function is executed using function invocation, its 'this' value will be undefined.

function wsmanFirmwareUpdateJob(options, context, taskId) {
wsmanFirmwareUpdateJob.super_.call(this, logger, options, context, taskId);

this.nodeId = this.context.target;

Choose a reason for hiding this comment

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

If a strict mode function is executed using function invocation, its 'this' value will be undefined.

* @constructor
*/
function wsmanFirmwareUpdateJob(options, context, taskId) {
wsmanFirmwareUpdateJob.super_.call(this, logger, options, context, taskId);

Choose a reason for hiding this comment

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

If a strict mode function is executed using function invocation, its 'this' value will be undefined.


describe('handle cases for _handleAsyncRequest function', function(){
beforeEach(function() {
job.wsman = new WsmanTool();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

WSMANTOOL init parameter

var self = this;

self.dellConfigs = configuration.get('dell');
if (!self.dellConfigs || !self.dellConfigs.gateway) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

dellConfigs.firmware

@nortonluo nortonluo force-pushed the wsmanfirmware branch 2 times, most recently from ceeaacf to ebea69b Compare November 6, 2017 08:53
@@ -0,0 +1,204 @@
//Copyright 2016, EMC, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

Copyright

if ( !(item.status === 'undefined' || item.status === null || item.status === ""|| failureFound === true)){
if(item.status === 'Failed') {
isSuccessful = false;
failureFound = true;
Copy link
Member

Choose a reason for hiding this comment

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

what's the difference with isSuccessful?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the array has multiple result field only isSussessful is insufficient to judge. Add a "failurlfound" flag just like a break operation(break could not be used in foreach).

@@ -0,0 +1,16 @@
// Copyright 2016, EMC, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

copyright


/*
* Initialize basic configuration for the job
*
Copy link
Member

Choose a reason for hiding this comment

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

unused '*'

{
var logger = Logger.initialize(wsmanFirmwareUpdateJobFactory);
/**
*
Copy link
Member

Choose a reason for hiding this comment

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

unused '*'

@anhou
Copy link
Member

anhou commented Nov 7, 2017

Please squash the commits

* @constructor
*/
function wsmanFirmwareUpdateJob(options, context, taskId) {
var self = this;

Choose a reason for hiding this comment

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

If a strict mode function is executed using function invocation, its 'this' value will be undefined.

@nortonluo nortonluo force-pushed the wsmanfirmware branch 2 times, most recently from 7ec98c5 to 9725e80 Compare November 7, 2017 10:17
*/
function wsmanFirmwareUpdateJob(options, context, taskId) {
wsmanFirmwareUpdateJob.super_.call(this, logger, options, context, taskId);
var self = this;

Choose a reason for hiding this comment

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

If a strict mode function is executed using function invocation, its 'this' value will be undefined.

Refine Format

Refine based on hound

Refine format

Remove debug info

Refine code based on review comments

Refine this to self

Fix conflict

Fix conflict

Retest CI

Refine copyrights fields

Change copyrights

Fix this ->self

Remove debug

Fix hound

Fix hound message
@anhou anhou merged commit 50d776b into RackHD:master Nov 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants