Skip to content

Conversation

@danilobuerger
Copy link
Contributor

@danilobuerger danilobuerger commented Sep 12, 2022

ignore strategies like git, expect directories to be passed in with a trailing slash in order to properly match.

consider a source tree:

├── hello
│   └── index.html
├── index.html

now we want to exclude everything except all html files (including all subdirectories).

this can be done with a gitignore of:

*
!*/
!*.html

in order for the git ignore strategy to properly match this, it needs to know that hello is a directory.

this patch just does that by appending a trailing slash for the ignore strategies.

Fixes #9146


All Submissions:

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

@gitpod-io
Copy link

gitpod-io bot commented Sep 12, 2022

@aws-cdk-automation aws-cdk-automation requested a review from a team September 12, 2022 12:14
@github-actions github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p2 labels Sep 12, 2022
@danilobuerger
Copy link
Contributor Author

The build failure seems unrelated. At least I can't find the cause from the logs thats related to my code change.

const sourceFilePath = path.join(srcDir, file.name);

if (ignoreStrategy.ignores(sourceFilePath)) {
const ignorePath = sourceFilePath + (file.name && file.isDirectory() ? path.sep : "");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build failure message:

@aws-cdk/core: /codebuild/output/src689345531/src/github.com/aws/aws-cdk/packages/@aws-cdk/core/lib/fs/copy.ts
@aws-cdk/core:   22:87  error  Strings must use singlequote  quotes


let relativePath = path.relative(this.absoluteRootPath, absoluteFilePath);
if (relativePath) {
relativePath += absoluteFilePath.endsWith(path.sep) ? path.sep : "";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Build failure message:

@aws-cdk/core: /codebuild/output/src689345531/src/github.com/aws/aws-cdk/packages/@aws-cdk/core/lib/fs/ignore.ts
@aws-cdk/core:   121:72  error  Strings must use singlequote  quotes
@aws-cdk/core:   181:72  error  Strings must use singlequote  quotes
@aws-cdk/core:   227:72  error  Strings must use singlequote  quotes

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review September 12, 2022 14:53

Pull request has been modified.

@danilobuerger danilobuerger changed the title Make sure ignore strategies receive directories with trailing slash fix(core): make sure ignore strategies receive directories with trailing slash Sep 12, 2022
Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests)

PRs with changes that are not statically guaranteed by the type system must have tests to make sure that (a) the new behavior works as intended and (b) it is not accidentally undone by future changes (See Contributing Guide, Work Your Magic (https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#step-3-work-your-magic)).

@danilobuerger
Copy link
Contributor Author

Hi @TheRealAmazonKendra, thanks for your helpful feedback! I really appreciate it. Sorry I didn't see the failures in the build log. I will try to get some meaningful tests in.

@danilobuerger danilobuerger force-pushed the ignoredirs branch 2 times, most recently from e1395cd to 36326dd Compare September 12, 2022 15:32
@danilobuerger
Copy link
Contributor Author

@TheRealAmazonKendra it looks like its all green now. Thanks again for all your help on getting me there!

…ing slash

ignore strategies like git, expect directories to be passed in with a
trailing slash in order to properly match.

consider a source tree:

├── hello
│   └── index.html
├── index.html

now we want to exclude everything except all html files
(including all subdirectories).

this can be done with a gitignore of:
*
!*/
!*.html

in order for the git ignore strategy to properly match this,
it needs to know that hello is a directory.

this patch just does that by appending a trailing slash for the
ignore strategies.
@TheRealAmazonKendra
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Oct 31, 2022

update

✅ Branch has been successfully updated

@TheRealAmazonKendra TheRealAmazonKendra changed the title fix(core): make sure ignore strategies receive directories with trailing slash fix(core): ignore strategies do not match directories Oct 31, 2022
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 891b5bb
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@aws-cdk-automation
Copy link
Collaborator

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

@aws-cdk-automation
Copy link
Collaborator

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

@aws-cdk-automation aws-cdk-automation added the closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. label Nov 30, 2022
@aws-cdk-automation
Copy link
Collaborator

The pull request linter fails with the following errors:

❌ Fixes must contain a change to an integration test file and the resulting snapshot.

PRs must pass status checks before we can provide a meaningful review.

mergify bot pushed a commit that referenced this pull request Sep 22, 2025
…ncluding matched files (#35511)

### Issue # (if applicable)

Closes #9146

### Reason for this change

When bundling assets with `IgnoreMode.GIT`, files that should be included via negation patterns (like `!*.html`) are incorrectly excluded if they're inside directories that are also re-included by negation patterns. Here's an example - consider the following tree:
```
- index.html
- app/
    - component.js
    - index.html
    - home.html
```

If we use a `exclude` pattern of `['*', '!*.html', '!*/']`, the `index.html` in the root and both HTML files from the `app` folder should be included - the `app` folder shouldn't be excluded, due to the `!*/` pattern. Right now, only the `index.html` file in the root folder is included.

### Description of changes

Similar to #22002, I've updated the `completelyIgnores` logic that's used to check if we need to skip a directory tree to include a trailing slash before verifying whether it's an ignored pattern. This makes it properly match directory-specific negation patterns like `!*/` by ensuring we're checking directory paths (with trailing slashes) against directory patterns.

I've also updated the logic in `fingerprint` to check if we're ignoring directories or files, and call `completelyIgnores` or `ignores` accordingly.

### Describe any new or updated permissions being added


None.

### Description of how you validated changes


Added unit tests covering the described scenarios.

### Checklist
- [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug This issue is a bug. closed-for-staleness This issue was automatically closed because it hadn't received any attention in a while. effort/small Small work item – less than a day of effort p2

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[@aws-cdk/assets] docs issue - Negate exclude pattern doesn't work

3 participants