-
Notifications
You must be signed in to change notification settings - Fork 4.2k
feat(core): add include
property specifying particular files for assets
#33132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This review is outdated)
3719ec5
to
8eed19e
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33132 +/- ##
==========================================
+ Coverage 83.98% 84.04% +0.06%
==========================================
Files 120 121 +1
Lines 6976 7003 +27
Branches 1178 1187 +9
==========================================
+ Hits 5859 5886 +27
Misses 1005 1005
Partials 112 112
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing To prevent automatic closure:
This PR will automatically close in 7 days if no action is taken. |
hi @go-to-k, what's the status on this PR? i don't want to rush you but the automation may eventually close your PR as abandoned... |
hi @kaizencc . Thanks for letting me know! |
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
// eslint-disable-next-line @typescript-eslint/no-require-imports | ||
const minimatch = require('minimatch'); | ||
|
||
export function matchIncludePatterns(patterns: string[], absoluteRootPath: string, absoluteFilePath: string): boolean { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost the same process as below.
https://github.com/go-to-k/aws-cdk/blob/9479e5ec9327870ec9791eb468c2ae5fdc76d9a5/packages/aws-cdk-lib/core/lib/fs/ignore.ts#L114-L137
* If matching the symlink but not its target, it is included (i.e. the `include` patterns match | ||
* the symlink path itself, regardless of its target). This is the same behavior even if `follow` | ||
* is not `SymlinkFollowMode.NEVER`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To match the current exclude behavior
const destDirPath = path.dirname(destFilePath); | ||
if (!fs.existsSync(destDirPath)) { | ||
fs.mkdirSync(destDirPath, { recursive: true }); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This directory creation code was added because:
- Previously, parent directories were already created by the
isDirectory()
processing before copying files/symlinks (*1). - With the
include
option to copy only specific files, directories that don't match the include patterns are not created (*2). Without this behavior, we would end up creating unnecessary directories along the traversal path when we finally reach files that aren't included. As a result, we need to explicitly create parent directories for included files/symlinks, particularly to handle cases where those parent directories aren't included.
*1
if (stat && stat.isDirectory()) {
fs.mkdirSync(destFilePath);
copyDirectory(sourceFilePath, destFilePath, options, rootDir);
stat = undefined;
}
*2
if (stat && stat.isDirectory()) {
if (included) {
fs.mkdirSync(destFilePath, { recursive: true });
}
copyDirectory(sourceFilePath, destFilePath, options, rootDir);
stat = undefined;
I've completed the necessary implementation, so I'm ready for review. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Hi @go-to-k ! I appreciate the effort put into identifying and addressing a real pain point with file filtering in CDK. As a reviewer of the last PR, I still have some concerns about the current implementation approach that I'd like to share: I see some potential issues with adding
Instead of modifying the core API, have you considered creating an extension class for your project? This would provide the same functionality with more simplified code. (Yes, it does not directly work with Lambda class YourAsset extends Asset {
constructor(scope: Construct, id: string, props: {
include: string[]; // New include option
} & AssetProps) {
// Transform include patterns into exclude negation patterns.
// This only works when exclude always takes precedence over include, but hides the ordering concerns.
const transformedExclude = [
'*',
...props.include.map(pattern => `!${pattern}`),
...(props.exclude ?? [])
]
super(scope, id, {
...props,
exclude: transformedExclude
});
}
} Although I don't think this wrapper should live in the aws-cdk-lib core module, it requires less code and offerrs the syntactic convenience you're looking for. What do you think? |
Hi @tmokmss . Thanks for your comments! I understand your concerns, but let me provide some replies: 1. Modal behaviorAre you arguing for the unnecessariness of If it's the former case, this discussion would not be limited to just this PR, but would concern include/exclude filtering itself. Include/exclude filtering has been introduced in "cdk watch" and other features in CDK, and it should be a common and natural feature that has been adopted in various software outside of CDK as well. Also, "cdk watch" in particular is a feature deeply related to assets, and I think it would be a great benefit to be able to align asset filtering settings with "cdk watch" settings. If it's the latter case, I would appreciate it if you could share any good suggestions you might have. (However, "cdk watch" has the same logic, so I think it would be good to match "cdk watch".) 2. Fixed priority limitationsThis same limitation exists in Maintaining consistency with the existing Also, in that case, using exclude without include would be good even if adding include. 3. Unnecessary forced decisionsRegarding questions about precedence and behavior when using both options, these are standard documentation challenges that can be resolved through clear API documentation and examples. Choosing between Also,
Thank you for the good example. However, if I only consider my project, I can handle it in any way. It's already possible to handle it with exclude alone. Rather, I think it should be introduced into the CDK library. |
@go-to-k Thanks for your detailed response. I have a few additional thoughts:
While I don't think the feature is completely unnecessary, I question whether the benefits outweigh the added complexity and potential confusion. This is ultimately a maintainer decision, but I'd ask them to consider if this feature - which works well only for specific use cases - belongs in such a fundamental construct. I acknowledge your point about cdk watch having the same limitations, but that's a separate consideration. I appreciate your dedication to improving the CDK developer experience. These are just my thoughts as a reviewer to help the maintainers make an informed decision. Thanks! |
I appreciate your response. |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #26107.
Reason for this change
Currently the option for Assets etc. has the
exclude
only for filtering files. Also I had submitted the PR that addsinclude
before, but closed it because theexclude
can cover situations for staging particular files by using negation patterns such as*
and!filenames
. (see: #26107 (comment)).But I encountered the situation that it is difficult to express our requirements with
exclude
only.The example of directories and files is the following (y: required, x: not required).
Now for the situation, we may use the following pattern with
exclude
only.However, there are some confusing points if the double negative is desired:
!
) and positive are mixed and difficult to understand.['*', 'subdir/examples/*', '!main.py', '!subdir', '!subdir/**/*']
In such cases, the
include
option makes it simple to achieve the following. (This is very easy to understand and you don't have to mind the order.)The
include
property is also present in other modules that filter particular files, such ascdk watch
, codepipeline Git triggers and BucketDeployment for S3, so it isn't unnatural.In particular, it should be good for developers to be able to match the filtering settings of
cdk watch
with hotswap and the settings of the asset files added in this PR.Description of changes
include
parameter to interface FileOptions as well asexclude
.include
in copyDirectory, where the actual file search is done.include
andexclude
,exclude
takes precedence.Describe any new or updated permissions being added
Description of how you validated changes
Both unit and integ tests.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license