fix(assets): support exceptions to exclude patterns #4473

nmussy · 2019-10-11T12:59:32Z

Allows adding back files and directories from a previously excluded directory. Previous implementations of copyDirectory and fingerprint stopped walking through the directory if it was ignored (see comment in #4450)

expand dir exclusions into dir/*, to allow minimist to match files in folder
add listFilesRecursively method, removing duped code from copy and fingerprint
add additional .dockerignore test cases into @aws-cdk/aws-ecr-assets
refactor existing test cases

Fixes #4450

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

* additional test cases * refactor existing tests

mergify · 2019-10-11T13:00:13Z

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

PR title type(scope): text
- type: fix, feat, refactor go into CHANGELOG, chore is hidden
- scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
- text: use all lower-case, do not end with a period, do not include issue refs
PR Description
- Rationale: describe rationale of change and approach taken
- Issues: indicate issues fixed via: fixes #xxx or closes #xxx
- Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
Testing
- Unit test added. Prefer to add a new test rather than modify existing tests
- CLI or init templates change? Re-run/add CLI integration tests
Documentation
- README: update module README to describe new features
- API docs: public APIs must be documented. Copy from official AWS docs when possible
- Design: for significant features, follow design process

aws-cdk-automation · 2019-10-11T13:41:50Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-10-11T15:03:54Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

packages/@aws-cdk/assets/lib/fs/copy.ts

aws-cdk-automation · 2019-10-11T19:35:32Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-10-21T10:18:58Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

packages/@aws-cdk/assets/lib/fs/utils.ts

rix0rrr · 2019-10-21T10:10:49Z

packages/@aws-cdk/assets/lib/fs/utils.ts

+    if (stat.isDirectory()) {
+      // to help future shouldExclude calls, we're changing the exlusion patterns
+      // by expliciting "dir" exclusions to "dir/*" (same with "!dir" -> "!dir/*")
+      exclude = exclude.reduce<string[]>((res, pattern) => {


I don't think this should be done in-line while traversing. It is also possible to run through the list of entries in the ignore list, see which ones are directories and treat those as prefixes.

In fact, I'd like to convert the "list of exclude/include entries" into a function or class that encapsulates the behavior as soon as possible and pass that around.

I'm thinking something like:

const ignoreList = parseIgnoreList(rootDirectory, ...list of strings);

ignoreList will be either of type (path: string) => boolean, or be a class with a method of that type.

I would have that as an argument to listFilesRecursively().

The issue is that it wouldn't be very economical, fs calls-wise. I'm filling the exclude directory on the fly because I'm also checking the stat of the files I'm traversing.

packages/@aws-cdk/aws-ecr-assets/lib/image-asset.ts

rix0rrr · 2019-10-21T10:18:06Z

packages/@aws-cdk/aws-ecr-assets/test/dockerignore-image-advanced/.dockerignore

@@ -0,0 +1,22 @@
+# This a comment, followed by an empty line


Can we do these tests in memory?

I'm thinking of a test where we have preloaded this list of ignore patterns, and pushing a number of paths past it (either by means of shouldExclude() or by means of a new IgnoreList API)

That is effectively the same and easier to change/assert against than making a whole new fixture with "just these" files in it.

I ended up creating a createFsStructureFromTree function, and removing the previous fixtures (with the exception of demo-image/).

Does that work for you?

packages/@aws-cdk/assets/lib/fs/utils.ts

packages/@aws-cdk/assets/lib/fs/copy.ts

packages/@aws-cdk/assets/lib/fs/utils.ts

aws-cdk-automation · 2019-10-21T11:41:26Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-10-21T11:47:19Z

AWS CodeBuild CI Report

Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-10-21T12:42:37Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

rix0rrr · 2019-10-29T09:42:29Z

packages/@aws-cdk/assert/lib/fs-utils.ts

@@ -0,0 +1,98 @@
+import fs = require('fs');


I'm not sure whether it's necessary to upgrade this to a feature of the assert library yet, especially since it doesn't really assert anything, will not be used by most of the construct libraries, and will never be JSII-able.

At least we can just keep this as a testing utility in the assets library until we definitely need it somewhere else, no?

I moved it to assert because I was using it both in assets and aws-ecr-assets. I could move it to assets instead, and expose it to make to available to aws-ecr-assets?

I see. Yes, I think put it in assets, and in fact you could import it directly from the test/ directory in aws-ecr-assets:

import fsUtils = require('@aws-cdk/assets/test/fs-utils');

It's not super nice, but I don't want test helpers to be part of the public API of a package. This usage would be okay by me since it's between 1st-party packages only. Would be nice to spend a couple of comment lines on it when you're making that change.

rix0rrr · 2019-10-29T09:50:27Z

packages/@aws-cdk/assets/lib/fs/utils.ts

+ * @returns `true` if the file should be excluded, followed by the index of the rule applied
+ */
+export function shouldExcludePriority(exclude: string[], filePath: string): [boolean, number] {
+  return exclude.reduce<[boolean, number]>((res, pattern, patternIndex) => {


I know reduce is sexy to use, but since you're not even using current accumulation res in your reducer, I don't really see the value. What's wrong with a simple for loop that returns the last set value?

I was originally using a map to have both pattern and patternIndex. I'll revert back to it.

rix0rrr · 2019-10-29T09:51:35Z

packages/@aws-cdk/assets/lib/fs/utils.ts

+ *
+ * @returns `true` if the file should be excluded, followed by the index of the rule applied
+ */
+export function shouldExcludePriority(exclude: string[], filePath: string): [boolean, number] {


Does this function really need to be exported?

The name confuses me. Proposed name change: how about evaluateExcludePatterns.

Or in fact, I've been saying this a couple of times already: please put these related functions that operate on the same data structure in a class. They're not as independent as you would like.

class ExcludeRules { constructor(private readonly patterns: string[]) { } public excludeFile(filePath: string): boolean { ... } public excludeDirectory(dirPath: string): boolean { ... } private evaluate(pathName: string): [boolean, number] { ... } }

I've switched to a class, that I'm still exporting to be able to test it. Same for your evaluate/my evaluateFile, it needs to be public.

rix0rrr · 2019-10-29T09:51:40Z

packages/@aws-cdk/assets/lib/fs/utils.ts

+ * @param exclude  exclusion patterns
+ * @param filePath file path to be assessed against the pattern
+ */
+export function shouldExcludeDeep(exclude: string[], relativePath: string): boolean {


Does this function really need to be exported?

I was exporting it to test it. It's the same now with the public method

rix0rrr · 2019-10-29T09:51:48Z

packages/@aws-cdk/assets/lib/fs/utils.ts

+ * @param exclude  exclusion patterns
+ * @param directoryPath directory path to be assessed against the pattern
+ */
+export function shouldExcludeDirectory(exclude: string[], directoryPath: string): boolean {


Does this function really need to be exported?

I was exporting it to test it. It's the same now with the public method

rix0rrr · 2019-10-29T10:00:47Z

packages/@aws-cdk/assets/lib/fs/utils.ts

+export function shouldExcludeDeep(exclude: string[], relativePath: string): boolean {
+  const [_shouldExclude] = relativePath.split(path.sep).reduce<[boolean, number, string]>(
+    ([accExclude, accPriority, pathIterator], pathComponent) => {
+      pathIterator = path.join(pathIterator, pathComponent);


I feel it would be cleaner if you made a function like pathComponents() (which given a path like a/b/c would return [a, a/b, a/b/c]) and iterated over that.

Again I feel a simple for loop would be more natural. If you really want to make it functional, why not something like:

const resultsAndPrios = pathComponents(relativePath).map(pc => shouldExcludePriority(rules, pc)); const [shouldExclude] = pickHighest(resultsAndPrios, x => x[1]); return shouldExclude;

Requires you to write some helpers, but to me is a lot clearer what is happening.

I did some refactoring, preferring for loops over reduces. I've only kept maps when I needed to know both the current array value and index.

rix0rrr · 2019-10-29T10:01:51Z

packages/@aws-cdk/assets/lib/fs/utils.ts

+
+/**
+ * Determines whether a given directory should be excluded and not explored further
+ * This might be true even if the directory is explicitly excluded,


In the case you're describing, shouldn't the function return false rather than `true?

rix0rrr · 2019-10-29T10:12:02Z

packages/@aws-cdk/assets/test/fs/test.utils.ts

+      test.done();
+    },
+    'contridactory'(test: Test) {
+      testShouldExcludeDeep(test, ['foo.txt', '!foo.txt'], [], ['foo.txt']);


How about contradictory with the order reversed?

rix0rrr · 2019-10-29T10:12:40Z

packages/@aws-cdk/assets/test/fs/test.utils.ts

@@ -192,4 +194,329 @@ export = {
      },
    }
  },
+
+  shouldExcludeDeep: {


Thanks for a thorough test suite!

aws-cdk-automation · 2019-10-29T14:21:34Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-10-29T14:25:46Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2019-10-30T10:48:50Z

Thank you for contributing! Your pull request is now being automatically merged.

nmussy · 2019-10-30T10:55:17Z

Thanks again @rix0rrr 🎉

aws-cdk-automation · 2019-10-30T11:32:18Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2019-10-30T11:40:41Z

AWS CodeBuild CI Report

Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

* chore: better test * fix: folder exclusion * additional test cases * refactor existing tests * chore: add new test files * fix: listFilesRecursively refactor (wip) * fix: finish refactoring listFilesRecursively * fix: implement mkdirpSync * fix: symlink discovery * fix: don't follow symlinks early * fix: create empty directories * chore: remove useless let * fix: symlink fingerprinting * fix: don't throw when fingerprinting directories * chore: remove unneeded 'exclude' cloning * chore: refactor mkdirp calls * chore: refactor AssetFile * chore: refactor recursion * chore: prevent unneeded stats call * chore: createFsStructureFromTree, remove empty files * chore: cleanup * fix: empty-directory test * feat: shouldExcludeDeep * chore: fromTree in @/assert, cleanup fn, test * feat: shouldExcludeDirectory * chore: refactor listFiles with new methods (missing symlinks) * feat: add symlink support to fromTree * fix: fromTree external directory symlink * fix: listFiles symlink support * chore: additional contridactory test * chore: fix docs * chore: ExcludeRules class refactor * chore: evaluateFile refactor * chore: further evaluateFile refactor * chore: evaluateDirectory refactor * chore: move FsUtils to assets * chore: move FsUtils to assets (unstaged files)

This reverts commit b7b4336.

…5022) This reverts commit b7b4336. This is being reverted as it introduced a regression in assets that contain symlinks during cdk synth. Fixes #4978

…nto 4295-windows-ecs-support * '4295-windows-ecs-support' of github.com:arhea/aws-cdk: chore(deps-dev): bump @types/lodash from 4.14.146 to 4.14.147 (aws#5021) Revert "fix(assets): support exceptions to exclude patterns (aws#4473)" (aws#5022) chore(deps): bump jsii-pacmak from 0.20.3 to 0.20.5 (aws#5003) chore(deps): bump codemaker from 0.20.3 to 0.20.5 (aws#5007) chore(deps-dev): bump @types/jest from 24.0.22 to 24.0.23 (aws#4993) chore(deps): bump jsii from 0.20.3 to 0.20.5 (aws#5006) chore(deps-dev): bump jsii-diff from 0.20.3 to 0.20.5 (aws#5005) chore(deps): bump jsii-spec from 0.20.3 to 0.20.5 (aws#5008) chore(core): resolve tokens before publishing tree.json (aws#4984) feat(cli): adding new option to `cdk deploy` to indicate whether ChangeSet should be executed (aws#4852) chore: move semantic.yaml to .github/ fix(core): unable to find stack by name using the cli in legacy mode (aws#4998) fix(ecs-patterns): Fix issue related to protocol being passed to target group (aws#4988)

Jimmy Gaussen added 4 commits October 11, 2019 10:31

chore: better test

3af6b91

fix: folder exclusion

e0738c3

* additional test cases * refactor existing tests

chore: add new test files

dce18c6

Merge remote-tracking branch 'upstream/master' into 4450

00c3852

fix: listFilesRecursively refactor (wip)

5faf3ca

SomayaB assigned rix0rrr Oct 11, 2019

Merge remote-tracking branch 'upstream/master' into 4450

d4cff4d

nmussy commented Oct 11, 2019

View reviewed changes

packages/@aws-cdk/assets/lib/fs/copy.ts Outdated Show resolved Hide resolved

fix: finish refactoring listFilesRecursively

89c792f

nmussy marked this pull request as ready for review October 11, 2019 19:50

nmussy requested a review from rix0rrr as a code owner October 11, 2019 19:50

Jimmy Gaussen added 2 commits October 21, 2019 11:57

Merge remote-tracking branch 'upstream/master' into 4450

9cc89c8

fix: implement mkdirpSync

13602a5

rix0rrr reviewed Oct 21, 2019

View reviewed changes

Jimmy Gaussen added 2 commits October 21, 2019 13:25

fix: symlink discovery

65a3f96

fix: don't follow symlinks early

6afead3

Jimmy Gaussen added 2 commits October 21, 2019 13:41

fix: create empty directories

3b745bd

chore: remove useless let

1462566

Jimmy Gaussen added 4 commits October 21, 2019 14:03

fix: symlink fingerprinting

5abcf66

fix: don't throw when fingerprinting directories

12d896e

chore: remove unneeded 'exclude' cloning

2172793

chore: refactor mkdirp calls

47bc251

rix0rrr reviewed Oct 29, 2019

View reviewed changes

Jimmy Gaussen added 10 commits October 29, 2019 11:57

Merge remote-tracking branch 'upstream/master' into 4450

455f912

chore: additional contridactory test

b49f64f

chore: fix docs

44f7757

chore: ExcludeRules class refactor

ab92e3f

chore: evaluateFile refactor

af2098b

chore: further evaluateFile refactor

9cf39b0

chore: evaluateDirectory refactor

02a8d0e

chore: move FsUtils to assets

23331a3

chore: move FsUtils to assets (unstaged files)

fc95caa

Merge remote-tracking branch 'upstream/master' into 4450

e2cee6b

rix0rrr changed the title ~~fix(assets): support exceptions in exclude patterns~~ fix(assets): support exceptions to exclude patterns Oct 30, 2019

rix0rrr approved these changes Oct 30, 2019

View reviewed changes

mergify bot added 2 commits October 30, 2019 10:49

Merge branch 'master' into 4450

75f7c3c

Merge branch 'master' into 4450

d2d9f43

mergify bot merged commit b7b4336 into aws:master Oct 30, 2019

nmussy deleted the 4450 branch October 30, 2019 11:46

nakedible mentioned this pull request Oct 30, 2019

Allow single files as Lambda code assets #4428

Closed

2 tasks

eladb mentioned this pull request Oct 31, 2019

Lambda: make it easier to specify a list of files and directories instead of a directory for code assets #4776

Closed

shivlaks added a commit that referenced this pull request Nov 14, 2019

Revert "fix(assets): support exceptions to exclude patterns (#4473)"

58379e7

This reverts commit b7b4336.

shivlaks mentioned this pull request Nov 14, 2019

cdk synth fails to resolve symlinks when staging assets as of 1.16.0 #4978

Closed

fix(assets): support exceptions to exclude patterns #4473

fix(assets): support exceptions to exclude patterns #4473

Conversation

nmussy commented Oct 11, 2019 • edited Loading

mergify bot commented Oct 11, 2019

aws-cdk-automation commented Oct 11, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 11, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 11, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 21, 2019

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 21, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 21, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 21, 2019

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 29, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 29, 2019

AWS CodeBuild CI Report

mergify bot commented Oct 30, 2019

nmussy commented Oct 30, 2019

aws-cdk-automation commented Oct 30, 2019

AWS CodeBuild CI Report

aws-cdk-automation commented Oct 30, 2019

AWS CodeBuild CI Report

nmussy commented Oct 11, 2019 •

edited

Loading