adding package cli command #422
Conversation
|
cc @matteofigus 😄 |
|
Thanks @debopamsengupta sorry for taking some time. Perhaps @mattiaerre can help a bit with the review? If not I'll have a look tomorrow :) |
|
No problem, thanks @matteofigus :) |
There was a problem hiding this comment.
thanks @debopamsengupta for this PR; I added some comments so let me know if they make sense to you. Happy to jump on a Google Hangouts meeting so that we can review them together; btw I've been able to pull your PR locally and run the CLI and it looks great. Very good job!
// cc @matteofigus
src/cli/facade/package.js
Outdated
| var path = require('path'); | ||
|
|
||
| module.exports = function(dependencies) { | ||
| var registry = dependencies.registry, |
There was a problem hiding this comment.
[eslint] 'registry' is assigned a value but never used. (no-unused-vars)
src/cli/facade/package.js
Outdated
| var componentPath = opts.componentPath, | ||
| packageDir = path.resolve(componentPath, '_package'), | ||
| compressedPackagePath = path.resolve(componentPath, 'package.tar.gz'), | ||
| errorMessage; |
There was a problem hiding this comment.
[eslint] 'errorMessage' is defined but never used. (no-unused-vars)
test/unit/cli-facade-package.js
Outdated
|
|
||
| var logSpy = {}, | ||
| Registry = require('../../src/cli/domain/registry'), | ||
| registry = new Registry(), |
There was a problem hiding this comment.
You do not need the registry here as well
| }); | ||
| }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
also having a look at the runner's report I see that the way the tests have been nested is probably not correct:
also running a coverage report it looks like some lines are missing from your tests:
package.js | 90.63 | 83.33 | 85.71 | 90.63 | 16,43,44 |There was a problem hiding this comment.
@mattiaerre should have improved now :) also added some extra strings for success messages
test/unit/cli-facade-package.js
Outdated
| Local = require('../../src/cli/domain/local'), | ||
| local = new Local({ logger: { log: function(){} } }), | ||
| readStub = sinon.stub().yields(null, 'test'), | ||
| PackageFacade = injectr('../../src/cli/facade/package.js', { read: readStub }), |
There was a problem hiding this comment.
you don't really need injectr here; you can just require package
|
@mattiaerre thanks for the comments :) made some improvements on the tests |
|
hi @debopamsengupta thanks for updating this PR; now we have got also 100% code coverage on that component which is very good. So this LGTM 👍 |
|
Thanks for working on this @debopamsengupta, very appreciated! |
Adding the
oc packageCLI command as per #419Also added a
--compressoption to generate thepackage.tar.gzfile