Allow multiple entrypoints to be provided and create zip files for each by philidem · Pull Request #7 · lifeomic/lambda-tools

philidem · 2018-06-08T00:32:28Z

The code is now more flexible about entrypoints and creating zip files for each. Before my change there was a limitation that did not allow you to use the -z option if more than one entrypoint is provided.

I decided to do our own zipping instead of using ZipPlugin for weback so that there is one code path for creating zip files.

Also, if an entrypoint is a directory then the directory is automatically expanded into multiple entrypoints by scanning the files in the directory.

…entrypoints and also allow all of these entrypoints to be zipped

…ypoint directories

mdlavin

Is there a usecase that these changes enable that was not possible by listing the entries points manually? The reason I ask is that there is a lot of change in here, including re-implementing zip creation, and it feels like somewhat of a bad practice to have too many entrypoints in the the same bundle anyway. I'm guessing I don't fully understand your usecase.

mdlavin · 2018-06-08T11:25:46Z

package.json

 {
  "name": "@lifeomic/lambda-tools",
-  "version": "3.0.4",
+  "version": "3.1.0",


I often wait to bump the version of the project until the functional changes are merged so that it avoid possible conflicts with updates while this is still under review

Fixed. I reverted this change.

Awesome, thank you!

mdlavin · 2018-06-08T11:27:58Z

src/webpack.js

+ * @param {String} outputDir the directory that contains output files
+ * @param {String[]} entryNames the entrypoint names from which output was produced
+ */
+async function zipOutputFiles (outputDir, entryNames) {


It's really a bummer that all this logic needs to be added to zip files. Re-using a plugin like ZipPlugin allowed us to avoid maintaining this code. Is it really not possible to re-use other peoples code? Even spending an hour or so learning or patching ZipPlugin would be better than maintaining this for the rest of time =)

There will be projects that have multiple lambdas but, also, by having support for ./src/lambas as input, you can more easily copy scripts/configuration from project to project and you will have better consistency. I like having a convention that you put your lambda functions at ./src/lambas so we can easily teach this and suggest a single build command.

mdlavin · 2018-06-08T11:29:21Z

bin/build.js

-  })
  .catch((error) => {
-    if (error.message === BREAKOUT) {
+    if (error.message === 'compilation_error') {


Did you mean to convert the old constant to a string literal? It seems like there is still logic in other places in the code that use this string and a constant would still be nice

The constant got split across files and I felt like the string was almost equivalent to use a shared constant.

jagoda · 2018-06-08T12:12:33Z

src/webpack.js

@@ -1,15 +1,18 @@
-const assert = require('assert');
+/* eslint-disable security/detect-object-injection */
+/* eslint-disable security/detect-non-literal-fs-filename */


Even though this is a development tool and therefore doesn't run in production I would personally still prefer that we not disable these rules for an entire file so that developers are encouraged to consider if each instance is really safe or necessary.

I can change this but I think these eslint rules are a little too strict for development tools.

Fixed (I now use eslint-disable-next-line <rule>)

Great, thanks!

jagoda · 2018-06-08T12:33:07Z

src/webpack.js

+        return candidateFile;
+      }
+    } catch (ignoreErr) {
+      // ignore error


Is it really safe to ignore the error here? This seems like it could hide real problems from the consumer like lack of access to the directory.

I think the catch with ignore is enough... the developer can dig deeper and look at the warning if no index file is found to further investigate.

Unless I misread the code, there is no warning. Rather the entry is silently ignored. I also disagree that a generic warning is sufficient. The only error that seems safe to ignore is if the file doesn't exist. Real I/O errors should be reported to the user.

Sorry, found the warning message right below. Still we need details as to why. It would be confusing to see "no index file was found" and see an index file in the directory.

I added a check for err.code !== 'ENOENT' and log warning with actual error message if we fail to stat a candidate index file.

Cool, works for me. Thanks!

jagoda · 2018-06-08T12:37:25Z

src/zip.js

@@ -0,0 +1,30 @@
+/* eslint-disable security/detect-non-literal-fs-filename */


Same as above.

jagoda · 2018-06-08T12:46:45Z

test/webpack-build.test.js

-  };
+  });
+
+  await fs.mkdirp(path.join(__dirname, 'fixtures/multi-lambdas/empty'));


This seems like it should be a permanent part of the fixtures directory or this test should operate on a temporary directory that is specific to this test case. Otherwise, other test cases will not be able to predictably account for this state change.

I can't checkin an empty directory and the test is specifically testing an empty directory.

So a temporary work directory specific to the test case or global test setup then? I just want to avoid potentially unexpected test results in the future because one test case is changing the fixtures directory contents during execution (and while multiple tests are running in parallel).

Ok. Good suggestion. I'll create a random directory with uuid and then delete it

Fixed. I create an empty directory whose name contains a uuid. I then delete the directory in a finally clause. Due to the nature of this test, the empty directory has to be nested under the ./test/fixtures/multi-lambdas directory.

I don't think that really addresses the issue since other multi-lambdas tests will still see the directory as state. I think you would need to create a copy of the multi-lambdas directory somewhere else to really address the issue.

jagoda · 2018-06-08T12:47:41Z

test/webpack-build.test.js

+  await fs.mkdirp(path.join(__dirname, 'fixtures/multi-lambdas/empty'));

-  await test.throws(build(options), /multiple entrypoints/i);
+  test.true(await fs.pathExists(path.join(test.context.buildDirectory, 'func1.js.zip')));


Stripping the directory from the entries on output seems potentially unsafe. What happens if something like lambda-tool-build -o buildDirectory ./func1.js ./fixtures/multi-lambdas is done? It seems to me that the contents of func1.js.zip would not be well-defined for the consumer in this case.

I don't think a change is necessary. I think the developer should be responsible for not producing lambda functions with the same name. I think webpack might detect that output files would be overridden as well. Also, I think you can produce two lambda output files with the same name before my changes.

Yes, you could do something like src/foo.js:bar.js src/baz.js:bar.js but I would agree in this case that it is the consumers fault. Without overriding the default naming it is not possible to create collisions since the file system will guarantee uniqueness. However, the case I mentioned is not as straight forward or obvious since the default behavior can create collisions without warning. My opinion is that if you provide a directory as an entry point then the output should be qualified with the same directory name by default so that the default "just works".

A nice enhancement would be to always warn on collisions, but that is outside the scope of this PR.

jagoda · 2018-06-08T13:30:04Z

src/webpack.js

+    const candidateFile = path.join(dir, indexFile);
+    try {
+      const stats = await fs.stat(candidateFile);
+      if (!stats.isDirectory()) {


This should probably be isFile(). I don't think we want to accept other non-regular file entries (like device files).

I wanted to also allow symbolic links as well and I felt like it was simpler and sufficient to just make sure it was not a directory. It is very unlikely that any other type of file will be created and if the user does manage to do that then they will see some other errors that will happen when we try to read the file.

I believe that fs.stat() follows symlinks.

…p files

coveralls · 2018-06-08T13:59:55Z

Pull Request Test Coverage Report for Build 50

75 of 75 (100.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 100.0%

Totals
Change from base Build 37:	0.0%
Covered Lines:	363
Relevant Lines:	363

💛 - Coveralls

…ng test

philidem · 2018-06-08T14:32:01Z

FYI, some of the builds are failing periodically due to yarnpkg timeouts (e.g. https://registry.yarnpkg.com/@babel/core/-/core-7.0.0-beta.49.tgz: ETIMEDOUT). Most builds do succeed though. 5 out of the 6 builds passed for the branch and PR after the last commit.

jagoda · 2018-06-08T16:26:01Z

test/webpack-build.test.js

+  test.true(await fs.pathExists(path.join(test.context.buildDirectory, 'lambda/service.js.zip')));
+});
+
+test.serial('Expand input entrypoint directory into multiple entrypoints', async (test) => {


Marking this as serial only causes it to be serialized wrt the other test cases in this file. This file will still be run in parallel with the other test files. This means that even though the directory name is unique and gets cleaned up, it still represents shared test state.

That's ok. There's a separate process for each test file so this should avoid interference caused by replacing fs.stat with a stub.

^ just to clarify, I added test.serial due to replacing fs.stat to simulate read error while searching for index file. I didn't want this temporary fs.stat replacement to interfere with other files.

Sorry, I should be more clear. This is the test case with the empty directory. My comment is in reference to that. I agree that fs.stat() should be fine.

philidem · 2018-06-11T16:35:47Z

@jagoda I think I have addressed your feedback. I'm going to merge and publish a new version so that we can start using these changes.

Phillip Gates-Idem added 2 commits June 7, 2018 19:38

Allow entrypoint to be a directory which gets expanded into multiple …

aa7d53e

…entrypoints and also allow all of these entrypoints to be zipped

Additional tests for outputting multiple zip files and expanding entr…

5c2eedc

…ypoint directories

philidem requested review from jagoda and mdlavin June 8, 2018 00:32

mdlavin suggested changes Jun 8, 2018

View reviewed changes

jagoda requested changes Jun 8, 2018

View reviewed changes

jagoda reviewed Jun 8, 2018

View reviewed changes

Changes based on PR feedback for handling multiple entrypoints and zi…

7294ef8

…p files

Add more console output for unreadable index file and add correspondi…

922c9c6

…ng test

jagoda requested changes Jun 8, 2018

View reviewed changes

mdlavin approved these changes Jun 8, 2018

View reviewed changes

philidem merged commit 275c886 into master Jun 11, 2018

jagoda deleted the Multi-Zip-Files branch August 7, 2018 15:20

		@@ -0,0 +1,30 @@
		/* eslint-disable security/detect-non-literal-fs-filename */

Conversation

philidem commented Jun 8, 2018

Uh oh!

mdlavin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philidem Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philidem Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

philidem Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jagoda Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 50

💛 - Coveralls

Uh oh!

philidem commented Jun 8, 2018

philidem Jun 8, 2018 •

edited

Loading

philidem Jun 8, 2018 •

edited

Loading

philidem Jun 8, 2018 •

edited

Loading

jagoda Jun 8, 2018 •

edited

Loading

coveralls commented Jun 8, 2018 •

edited

Loading