fix(GITHUB-6525-5172): Rewrite copyDirContentsSyncAllow to call fs-extra::copySync() on the directories instead of calling it on the files to copy individually

[stephaneseng]

What did you implement

The main motivation behind these changes is to rely on https://github.com/jprichardson/node-fs-extra/blob/8.1.0/docs/copy-sync.md as much as possible in order to avoid having to do the fullFilePath.replace(srcDir, '') operation because this operation can be error-prone.

Doing so fixes the following issues because the user-submitted file paths are now correctly interpreted by fs-extra:

Fixes #6525
Fixes #5172

How did you implement it

About noLinks

copyDirContentsSync() is called only once with a third parameter.

This parameter is set to { noLinks: true } at https://github.com/serverless/serverless/blob/v1.49.0/lib/plugins/create/create.js#L163.

Furthermore, noLinks is also the only option accepted by walkDirSync, as seen in https://github.com/serverless/serverless/blob/v1.49.0/lib/utils/fs/walkDirSync.js#L7.

Thus this noLinks option had to be kept in this new implementation, preventing the copy of symbolic links if it is set to true.

About dereference

In the previous implementation, when copied, symbolic links were always dereferenced.

This means that, when the following folder was copied:

./my-artifacts/
├── cloudformation-template-create-stack.json
├── cloudformation-template-update-stack.json
├── my-symbolic-link-to-serverless-state.json -> serverless-state.json
├── sandbox-serverless.zip
└── serverless-state.json

This resulting file structure was created, with my-symbolic-link-to-serverless-state.json no longer being a symbolic link:

./.serverless/
├── cloudformation-template-create-stack.json
├── cloudformation-template-update-stack.json
├── my-symbolic-link-to-serverless-state.json
├── sandbox-serverless.zip
└── serverless-state.json

The reason why that is the case is not obvious when reading https://github.com/jprichardson/node-fs-extra/blob/0.30.0/lib/copy-sync/copy-sync.js.

Even though the dereference option was always left to its false default value, the symbolic links were always dereferenced in our case because:

1. The files to copy were passed one by one to copySync()
2. When called with a file and not a directory, the dereference option is ignored by copySync(), as shown in https://github.com/jprichardson/node-fs-extra/blob/0.30.0/lib/copy-sync/copy-sync.js#L21

How can we verify it

For #6525:

$ serverless create --template aws-nodejs
$ serverless package --package ./my-artifacts
$ serverless deploy --package ./my-artifacts

For #5172: Specifically in a Windows environment:

$ serverless create --template aws-nodejs
$ serverless package --package ./my-artifacts/my-subfolder
$ serverless deploy --package ./my-artifacts/my-subfolder

Todos

Note: Run npm run test-ci to run all validation checks on proposed changes

• Write tests and confirm existing functionality is not broken.
  Validate via npm test
• Write documentation
• Ensure there are no lint errors.
  Validate via npm run lint-updated
  Note: Some reported issues can be automatically fixed by running npm run lint:fix
• Ensure introduced changes match Prettier formatting.
  Validate via npm run prettier-check-updated
  Note: All reported issues can be automatically fixed by running npm run prettify-updated
• Make sure code coverage hasn't dropped
• Provide verification config / commands / resources
• Enable "Allow edits from maintainers" for this PR
• Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

[medikoo]

@stephaneseng great thanks for that PR!

Relying on copySync is good call, however ignoring symlinks feels as a breaking change. Currently I believe they're resolved as files and copied so in destination we have files. While after taking this change in, they'll be missing.

Are we sure it wouldn't break existing scenarios?

[medikoo]

Can we move it to function signature as e.g. { noLinks = false } = {}? I think it'll be more readable (and less verbose) that way

[medikoo]

I think it'll be nicer to keep it within copySyncOptions definition as e.g.

 const copySyncOptions = {
    dereference: true,
    filter: options.noLinks ? isNotSymbolicLink : null
  };

[stephaneseng]

Thanks for your review @medikoo.
I have pushed an updated commit with your suggestions taken into account.

Concerning your question about the eventual breaking changes brought by these changes: The current behavior of copyDirContentsSync(srcDir, destDir, options) can be observed by executing the tests in the new file https://github.com/serverless/serverless/pull/6526/files#diff-605419039774c6becadd705ddbfb7048 on the master branch.

We can then observe the following behavior, on master:

1.a. When called with no options, or with options = { noLinks: false }:

./testSrc/
├── file1.txt
└── folder
    ├── file2.txt
    └── file3.txt -> /tmp/serverless/testSrc/folder/file2.txt

./testDest/
├── file1.txt
└── folder
    ├── file2.txt
    └── file3.txt

The symbolic link copied is indeed dereferenced.

1.b. When called with options = { noLinks: true }:

./testSrc/
├── file1.txt
└── folder
    ├── file2.txt
    └── file3.txt -> /tmp/serverless/testSrc/folder/file2.txt

./testDest/
├── file1.txt
└── folder
    └── file2.txt

The symbolic link is already ignored.

This is because:

• The call to walkDirSync() at: https://github.com/serverless/serverless/blob/v1.49.0/lib/utils/fs/copyDirContentsSync.js#L8...
• ... Does not return the file paths of symbolic links as these are ignored here, when options.noLinks = true: https://github.com/serverless/serverless/blob/v1.49.0/lib/utils/fs/walkDirSync.js#L21

Furthermore, by executing these tests on this PR branch, we can observe the same behavior than the current one:

2.a. When called with no options, or with options = { noLinks: false }:

./testSrc/
├── file1.txt
└── folder
    ├── file2.txt
    └── file3.txt -> /tmp/serverless/testSrc/folder/file2.txt

./testDest/
├── file1.txt
└── folder
    ├── file2.txt
    └── file3.txt

The symbolic link copied is also dereferenced.

2.b. When called with options = { noLinks: true }:

./testSrc/
├── file1.txt
└── folder
    ├── file2.txt
    └── file3.txt -> /tmp/serverless/testSrc/folder/file2.txt

./testDest/
├── file1.txt
└── folder
    └── file2.txt

The symbolic link is also ignored.

Thus, because the results of calls to copyDirContentsSync(srcDir, destDir, options) are the same before and after these changes, I do not think that these are breaking changes.

[medikoo]

@stephaneseng thanks for clarification. Indeed we're safe here, and it looks really good!

I really appreciate an extra tests that were introduced for that.

I'm taking this in, it'll be published tomorrow with v1.50.0