From 0a95913481e20a9743a1b090036f8de000748637 Mon Sep 17 00:00:00 2001 From: Mariusz Nowak Date: Mon, 10 Jul 2017 13:53:57 +0200 Subject: [PATCH 1/5] Seclude files resolution logic --- lib/plugins/package/lib/packageService.js | 84 +++++++++++++++---- .../package/lib/packageService.test.js | 46 ++++++---- lib/plugins/package/lib/zipService.js | 49 ++++------- lib/plugins/package/lib/zipService.test.js | 8 +- 4 files changed, 112 insertions(+), 75 deletions(-) diff --git a/lib/plugins/package/lib/packageService.js b/lib/plugins/package/lib/packageService.js index b232eaf19..533f77439 100644 --- a/lib/plugins/package/lib/packageService.js +++ b/lib/plugins/package/lib/packageService.js @@ -2,6 +2,7 @@ const BbPromise = require('bluebird'); const path = require('path'); +const globby = require('globby'); const _ = require('lodash'); module.exports = { @@ -59,19 +60,19 @@ module.exports = { }, packageAll() { - const exclude = this.getExcludes(); - const include = this.getIncludes(); const zipFileName = `${this.serverless.service.service}.zip`; - return this.zipService(exclude, include, zipFileName).then(filePath => { - // only set the default artifact for backward-compatibility - // when no explicit artifact is defined - if (!this.serverless.service.package.artifact) { - this.serverless.service.package.artifact = filePath; - this.serverless.service.artifact = filePath; - } - return filePath; - }); + return this.resolveFilePathsAll().then(filePaths => + this.zipFiles(filePaths, zipFileName).then(filePath => { + // only set the default artifact for backward-compatibility + // when no explicit artifact is defined + if (!this.serverless.service.package.artifact) { + this.serverless.service.package.artifact = filePath; + this.serverless.service.artifact = filePath; + } + return filePath; + }) + ); }, packageFunction(functionName) { @@ -93,15 +94,62 @@ module.exports = { return BbPromise.resolve(filePath); } - const exclude = this.getExcludes(funcPackageConfig.exclude); - const include = this.getIncludes(funcPackageConfig.include); const zipFileName = `${functionName}.zip`; - return this.zipService(exclude, include, zipFileName).then(artifactPath => { - functionObject.package = { - artifact: artifactPath, - }; - return artifactPath; + return this.resolveFilePathsFunction(functionName).then(filePaths => + this.zipFiles(filePaths, zipFileName).then(artifactPath => { + functionObject.package = { + artifact: artifactPath, + }; + return artifactPath; + }) + ); + }, + + resolveFilePathsAll() { + const params = { exclude: this.getExcludes(), include: this.getIncludes() }; + return this.excludeDevDependencies(params).then(() => + this.resolveFilePathsFromPatterns(params)); + }, + + resolveFilePathsFunction(functionName) { + const functionObject = this.serverless.service.getFunction(functionName); + const funcPackageConfig = functionObject.package || {}; + + const params = { + exclude: this.getExcludes(funcPackageConfig.exclude), + include: this.getIncludes(funcPackageConfig.include), + }; + return this.excludeDevDependencies(params).then(() => + this.resolveFilePathsFromPatterns(params)); + }, + + resolveFilePathsFromPatterns(params) { + const patterns = ['**']; + + params.exclude.forEach((pattern) => { + if (pattern.charAt(0) !== '!') { + patterns.push(`!${pattern}`); + } else { + patterns.push(pattern.substring(1)); + } + }); + + // push the include globs to the end of the array + // (files and folders will be re-added again even if they were excluded beforehand) + params.include.forEach((pattern) => { + patterns.push(pattern); + }); + + return globby(patterns, { + cwd: this.serverless.config.servicePath, + dot: true, + silent: true, + follow: true, + nodir: true, + }).then(filePaths => { + if (filePaths.length !== 0) return filePaths; + throw new this.serverless.classes.Error('No file matches include / exclude patterns'); }); }, }; diff --git a/lib/plugins/package/lib/packageService.test.js b/lib/plugins/package/lib/packageService.test.js index f3da7f193..e5e889c41 100644 --- a/lib/plugins/package/lib/packageService.test.js +++ b/lib/plugins/package/lib/packageService.test.js @@ -224,24 +224,29 @@ describe('#packageService()', () => { describe('#packageAll()', () => { const exclude = ['test-exclude']; const include = ['test-include']; + const files = []; const artifactFilePath = '/some/fake/path/test-artifact.zip'; let getExcludesStub; let getIncludesStub; - let zipServiceStub; + let resolveFilePathsFromPatternsStub; + let zipFilesStub; beforeEach(() => { getExcludesStub = sinon .stub(packagePlugin, 'getExcludes').returns(exclude); getIncludesStub = sinon .stub(packagePlugin, 'getIncludes').returns(include); - zipServiceStub = sinon - .stub(packagePlugin, 'zipService').resolves(artifactFilePath); + resolveFilePathsFromPatternsStub = sinon + .stub(packagePlugin, 'resolveFilePathsFromPatterns').returns(files); + zipFilesStub = sinon + .stub(packagePlugin, 'zipFiles').resolves(artifactFilePath); }); afterEach(() => { packagePlugin.getExcludes.restore(); packagePlugin.getIncludes.restore(); - packagePlugin.zipService.restore(); + packagePlugin.resolveFilePathsFromPatterns.restore(); + packagePlugin.zipFiles.restore(); }); it('should call zipService with settings', () => { @@ -254,10 +259,10 @@ describe('#packageService()', () => { .then(() => BbPromise.all([ expect(getExcludesStub).to.be.calledOnce, expect(getIncludesStub).to.be.calledOnce, - expect(zipServiceStub).to.be.calledOnce, - expect(zipServiceStub).to.have.been.calledWithExactly( - exclude, - include, + expect(resolveFilePathsFromPatternsStub).to.be.calledOnce, + expect(zipFilesStub).to.be.calledOnce, + expect(zipFilesStub).to.have.been.calledWithExactly( + files, zipFileName ), ])); @@ -267,24 +272,29 @@ describe('#packageService()', () => { describe('#packageFunction()', () => { const exclude = ['test-exclude']; const include = ['test-include']; + const files = []; const artifactFilePath = '/some/fake/path/test-artifact.zip'; let getExcludesStub; let getIncludesStub; - let zipServiceStub; + let resolveFilePathsFromPatternsStub; + let zipFilesStub; beforeEach(() => { getExcludesStub = sinon .stub(packagePlugin, 'getExcludes').returns(exclude); getIncludesStub = sinon .stub(packagePlugin, 'getIncludes').returns(include); - zipServiceStub = sinon - .stub(packagePlugin, 'zipService').resolves(artifactFilePath); + resolveFilePathsFromPatternsStub = sinon + .stub(packagePlugin, 'resolveFilePathsFromPatterns').returns(files); + zipFilesStub = sinon + .stub(packagePlugin, 'zipFiles').resolves(artifactFilePath); }); afterEach(() => { packagePlugin.getExcludes.restore(); packagePlugin.getIncludes.restore(); - packagePlugin.zipService.restore(); + packagePlugin.resolveFilePathsFromPatterns.restore(); + packagePlugin.zipFiles.restore(); }); it('should call zipService with settings', () => { @@ -301,11 +311,11 @@ describe('#packageService()', () => { .then(() => BbPromise.all([ expect(getExcludesStub).to.be.calledOnce, expect(getIncludesStub).to.be.calledOnce, + expect(resolveFilePathsFromPatternsStub).to.be.calledOnce, - expect(zipServiceStub).to.be.calledOnce, - expect(zipServiceStub).to.have.been.calledWithExactly( - exclude, - include, + expect(zipFilesStub).to.be.calledOnce, + expect(zipFilesStub).to.have.been.calledWithExactly( + files, zipFileName ), ])); @@ -329,7 +339,7 @@ describe('#packageService()', () => { .then(() => BbPromise.all([ expect(getExcludesStub).to.not.have.been.called, expect(getIncludesStub).to.not.have.been.called, - expect(zipServiceStub).to.not.have.been.called, + expect(zipFilesStub).to.not.have.been.called, ])); }); @@ -351,7 +361,7 @@ describe('#packageService()', () => { .then(() => BbPromise.all([ expect(getExcludesStub).to.not.have.been.called, expect(getIncludesStub).to.not.have.been.called, - expect(zipServiceStub).to.not.have.been.called, + expect(zipFilesStub).to.not.have.been.called, ])); }); }); diff --git a/lib/plugins/package/lib/zipService.js b/lib/plugins/package/lib/zipService.js index a3847d967..eefa78804 100644 --- a/lib/plugins/package/lib/zipService.js +++ b/lib/plugins/package/lib/zipService.js @@ -44,45 +44,26 @@ module.exports = { }, zip(params) { - const patterns = ['**']; + return this.resolveFilePathsFromPatterns(params).then(filePaths => + this.zipFiles(filePaths, params.zipFileName)); + }, - params.exclude.forEach((pattern) => { - if (pattern.charAt(0) !== '!') { - patterns.push(`!${pattern}`); - } else { - patterns.push(pattern.substring(1)); - } - }); - - // push the include globs to the end of the array - // (files and folders will be re-added again even if they were excluded beforehand) - params.include.forEach((pattern) => { - patterns.push(pattern); - }); + zipFiles(files, zipFileName) { + if (files.length === 0) { + const error = new this.serverless.classes.Error('No files to package'); + return BbPromise.reject(error); + } const zip = archiver.create('zip'); // Create artifact in temp path and move it to the package path (if any) later const artifactFilePath = path.join(this.serverless.config.servicePath, '.serverless', - params.zipFileName + zipFileName ); this.serverless.utils.writeFileDir(artifactFilePath); const output = fs.createWriteStream(artifactFilePath); - const files = globby.sync(patterns, { - cwd: this.serverless.config.servicePath, - dot: true, - silent: true, - follow: true, - }); - - if (files.length === 0) { - const error = new this.serverless - .classes.Error('No file matches include / exclude patterns'); - return BbPromise.reject(error); - } - output.on('open', () => { zip.pipe(output); @@ -94,13 +75,11 @@ module.exports = { const stats = fs.statSync(fullPath); - if (!stats.isDirectory(fullPath)) { - zip.append(fs.readFileSync(fullPath), { - name: filePath, - mode: stats.mode, - date: new Date(0), // necessary to get the same hash when zipping the same content - }); - } + zip.append(fs.readFileSync(fullPath), { + name: filePath, + mode: stats.mode, + date: new Date(0), // necessary to get the same hash when zipping the same content + }); }); zip.finalize(); diff --git a/lib/plugins/package/lib/zipService.test.js b/lib/plugins/package/lib/zipService.test.js index 37c79e14f..9adf161d1 100644 --- a/lib/plugins/package/lib/zipService.test.js +++ b/lib/plugins/package/lib/zipService.test.js @@ -287,7 +287,7 @@ describe('zipService', () => { expect(Object.keys(unzippedFileData) .filter(file => !unzippedFileData[file].dir)) - .to.be.lengthOf(13); + .to.be.lengthOf(12); // root directory expect(unzippedFileData['event.json'].name) @@ -368,7 +368,7 @@ describe('zipService', () => { expect(Object.keys(unzippedFileData) .filter(file => !unzippedFileData[file].dir)) - .to.be.lengthOf(8); + .to.be.lengthOf(7); // root directory expect(unzippedFileData['handler.js'].name) @@ -413,7 +413,7 @@ describe('zipService', () => { expect(Object.keys(unzippedFileData) .filter(file => !unzippedFileData[file].dir)) - .to.be.lengthOf(11); + .to.be.lengthOf(10); // root directory expect(unzippedFileData['event.json'].name) @@ -467,7 +467,7 @@ describe('zipService', () => { expect(Object.keys(unzippedFileData) .filter(file => !unzippedFileData[file].dir)) - .to.be.lengthOf(11); + .to.be.lengthOf(10); // root directory expect(unzippedFileData['event.json'].name) From 7f5d4426f63512e8b058dd3a96252311a7c29c86 Mon Sep 17 00:00:00 2001 From: Mariusz Nowak Date: Mon, 10 Jul 2017 20:08:17 +0200 Subject: [PATCH 2/5] Handle eventual write error --- lib/plugins/package/lib/zipService.js | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/plugins/package/lib/zipService.js b/lib/plugins/package/lib/zipService.js index eefa78804..4f4bc270c 100644 --- a/lib/plugins/package/lib/zipService.js +++ b/lib/plugins/package/lib/zipService.js @@ -87,6 +87,7 @@ module.exports = { return new BbPromise((resolve, reject) => { output.on('close', () => resolve(artifactFilePath)); + output.on('error', (err) => reject(err)); zip.on('error', (err) => reject(err)); }); }, From a24e243766d53ef1431adfdf3d156779c1d02ab5 Mon Sep 17 00:00:00 2001 From: Mariusz Nowak Date: Mon, 10 Jul 2017 20:17:14 +0200 Subject: [PATCH 3/5] Resolve files for archive asynchronously --- lib/plugins/package/lib/zipService.js | 46 +++++++++++++++------------ 1 file changed, 25 insertions(+), 21 deletions(-) diff --git a/lib/plugins/package/lib/zipService.js b/lib/plugins/package/lib/zipService.js index 4f4bc270c..4be5b1f89 100644 --- a/lib/plugins/package/lib/zipService.js +++ b/lib/plugins/package/lib/zipService.js @@ -11,6 +11,9 @@ const fs = require('fs'); const globby = require('globby'); const _ = require('lodash'); +const fsStat = BbPromise.promisify(fs.stat); +const fsReadFile = BbPromise.promisify(fs.readFile); + module.exports = { zipService(exclude, include, zipFileName) { const params = { @@ -64,31 +67,32 @@ module.exports = { const output = fs.createWriteStream(artifactFilePath); - output.on('open', () => { - zip.pipe(output); - - files.forEach((filePath) => { - const fullPath = path.resolve( - this.serverless.config.servicePath, - filePath - ); - - const stats = fs.statSync(fullPath); - - zip.append(fs.readFileSync(fullPath), { - name: filePath, - mode: stats.mode, - date: new Date(0), // necessary to get the same hash when zipping the same content - }); - }); - - zip.finalize(); - }); - return new BbPromise((resolve, reject) => { output.on('close', () => resolve(artifactFilePath)); output.on('error', (err) => reject(err)); zip.on('error', (err) => reject(err)); + + + output.on('open', () => { + zip.pipe(output); + + BbPromise.all(files.map((filePath) => { + const fullPath = path.resolve( + this.serverless.config.servicePath, + filePath + ); + + return fsStat(fullPath).then(stats => + fsReadFile(fullPath).then(content => + zip.append(String(content), { + name: filePath, + mode: stats.mode, + date: new Date(0), // necessary to get the same hash when zipping the same content + }) + ) + ); + })).then(() => zip.finalize()).catch(reject); + }); }); }, }; From 7d5b93696e250eb76676a530fdfdbad0a7194097 Mon Sep 17 00:00:00 2001 From: Mariusz Nowak Date: Mon, 10 Jul 2017 20:19:56 +0200 Subject: [PATCH 4/5] Expose getFileContent method --- lib/plugins/package/lib/zipService.js | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/plugins/package/lib/zipService.js b/lib/plugins/package/lib/zipService.js index 4be5b1f89..a38b44c3b 100644 --- a/lib/plugins/package/lib/zipService.js +++ b/lib/plugins/package/lib/zipService.js @@ -83,8 +83,8 @@ module.exports = { ); return fsStat(fullPath).then(stats => - fsReadFile(fullPath).then(content => - zip.append(String(content), { + this.getFileContent(fullPath).then(fileContent => + zip.append(fileContent, { name: filePath, mode: stats.mode, date: new Date(0), // necessary to get the same hash when zipping the same content @@ -95,6 +95,10 @@ module.exports = { }); }); }, + + getFileContent(fullPath) { + return fsReadFile(fullPath, 'utf8'); + }, }; function excludeNodeDevDependencies(servicePath) { From f4ee5481c6da0d61168bf1b2770958af6d571300 Mon Sep 17 00:00:00 2001 From: Mariusz Nowak Date: Mon, 10 Jul 2017 21:47:49 +0200 Subject: [PATCH 5/5] Test error handling in zipFiles --- lib/plugins/package/lib/zipService.test.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/plugins/package/lib/zipService.test.js b/lib/plugins/package/lib/zipService.test.js index 9adf161d1..8447a8125 100644 --- a/lib/plugins/package/lib/zipService.test.js +++ b/lib/plugins/package/lib/zipService.test.js @@ -508,4 +508,11 @@ describe('zipService', () => { .rejectedWith(Error, 'file matches include / exclude'); }); }); + + describe('#zipFiles()', () => { + it('should throw an error if no files are provided', () => + expect(packagePlugin.zipFiles([], path.resolve(__dirname, 'tmp.zip'))).to.be + .rejectedWith(Error, 'No files to package') + ); + }); });