-
Notifications
You must be signed in to change notification settings - Fork 77
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
RAC-6214 : add dell wsman delete volume #558
Conversation
lib/utils/job-utils/smb2-client.js
Outdated
Logger, | ||
Promise | ||
){ | ||
var logger = Logger.initialize(Smb2ClientFactory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'logger' is defined but never used.
lib/utils/job-utils/smb2-client.js
Outdated
}); | ||
XMLSerializer = XmlDOM.XMLSerializer; | ||
serializer = new XMLSerializer(); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary semicolon.
}).then(function(){ | ||
self._done(); | ||
}).catch(function(err){ | ||
logger.error('Error occurs', error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'error' is not defined.
return smb2Client.writeFile(fileName, doc); | ||
}).then(function(){ | ||
self._done(); | ||
}).catch(function(err){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'err' is defined but never used.
} | ||
} | ||
} | ||
for(var i = 0; i < deleteIndex.length; i++){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'i' is already defined.
Unexpected use of '++'.
var components = doc.getElementsByTagName('Component'); | ||
var deleteIndex = []; | ||
|
||
for(var i = 0; i < components.length; i++){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unexpected use of '++'.
1ca20cb
to
8aad9b5
Compare
8aad9b5
to
6d0400b
Compare
"description": "Specify if SSL certificate is enabled", | ||
"type": "boolean" | ||
}, | ||
"_taskTimeout": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an options for all tasks and it has been covered in common-task-options (https://github.com/RackHD/on-tasks/blob/master/lib/task-data/schemas/common-task-options.json#L22), so you don't need to describe it again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove "_taskTimeout"
"$ref": "#/definitions/shutdownType" | ||
} | ||
}, | ||
"required": ["volumeId"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if shutdownType is optional, you'd better tell its default value in its description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And there is difference between "setting a default value in task definition" and "describe it as optional in task schema". If the job needs this option for execution, then the option has to be required
no matter you set a default value in task definition or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add description for default value.
implementsTask: "Task.Base.Dell.Wsman.getComponent", | ||
optionsSchema: 'dell-wsman-delete-volume.json', | ||
options: { | ||
volumeId: "", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please confirm, is the empty volumeId
valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The volumeId check will be added.
lib/jobs/dell-wsman-RAID.js
Outdated
'validator' | ||
)); | ||
|
||
function DellWsmanRAIDFactory( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why no _run
callback?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_run callback is defined in it's parent class
lib/utils/job-utils/smb2-client.js
Outdated
} | ||
|
||
Smb2Client.prototype.readFile = function(fileName){ | ||
logger.info("Smb2Client readFile function, fileName:" + fileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use logger.debug
seems to be more reasonable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
} | ||
}; | ||
WsmanTool.prototype.clientRequest.resolves(response); | ||
expect(job.doRAIDoperation(obms)).to.be.fulfilled; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a promise it has to be returned, otherwise this test case is always success no matter the promise is fufilled or rejected.
validator.isIP.returns(false); | ||
expect(function(){ | ||
job.doRAIDoperation(obms); | ||
}).to.throw('Invalid ServerIP'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw
only works for sync function. For async function, use something like `return expect(...).to.be.rejected;'
configuration.get.returns(configFile); | ||
expect(function(){ | ||
job._run(); | ||
}).to.not.throw('Dell SCP web service is not defined in smiConfig.json.'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
throw
cannot be used for async function, it has been explained above.
lib/jobs/dell-wsman-getComponent.js
Outdated
|
||
WsmanGetComponentJob.prototype.getComponent = function(obm) { | ||
if (!validator.isIP(obm.config.host)) { | ||
throw new Error('Invalid ServerIP'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you design the function to return a promise, then you'd better also wrap the error in promise, you will find it will become easier to write unit-test.
lib/jobs/dell-wsman-getComponent.js
Outdated
var componentNames = ""; | ||
if(self.options.volumeId !== undefined){ | ||
componentNames = self.options.volumeId.split(':')[1]; | ||
}else if(self.options.drives !== undefined){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The task schema misses the description for the option drives
.
Meanwhile, look like this is a typo drives
==> 'drivers'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling error. It should be drivers. The option isn't used in delete volume. It will be defined in other feature, such as "add volume"
6d0400b
to
c2d38fb
Compare
|
||
module.exports = { | ||
friendlyName: 'Dell Wsman GetComponent Base Task', | ||
injectableName: 'Task.Base.Dell.Wsman.GetXml', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's deleted
Background
Replace RACADM based RAID operation feature implementation with WSMAN API. This pr is used for deleting volume operation.
Details
To solve this problem, a new graph is created which contains four tasks. The first task is used to getComponent xml file. The second task is used to update the xml file for deleting volume. The third task is used to delete volume. The last task is used to update mongodb. Details in https://rackhd.atlassian.net/browse/RAC-6214
Reviewers
@nortonluo @lanchongyizu @AlaricChan