Skip to content

fix(ecr-assets): incorrect handling of nested excludes in .dockerignore #34810

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(ecr-assets): incorrect handling of nested excludes in .dockerignore #34810

wants to merge 1 commit into from
+642 −439

Conversation

Abogical
Copy link
Member

@Abogical Abogical commented Jun 25, 2025 •
edited
Loading

Issue #13636

Closes #13636.

Reason for this change

Fixes a bug where asset copying does not correctly process .dockerignore files. CDK did not copy files excluded by a nested exclude pattern in .dockerignore.

Description of changes

Added a new method in DockerIgnoreStrategy that determines whether a directory is fully or partially ignored. The method is DockerIgnoreStrategy.completelyIgnores and if it returns true, it means that the directory is ignored AND that all of its children are ignored as well. For directories that are ignored but not "completely" ignored, the copyDirectory function will still walk through them.

As this changes the behaviour of asset copying, this can potientally be a breaking change. Therefore, a new feature flag is added which enables this fix. The feature flag is DOCKER_IGNORE_NESTED_EXCLUDE_FIX.

Describe any new or updated permissions being added

No new permissions are added.

Description of how you validated changes

Unit tests and integration tests are added.

Checklist

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

@Abogical Abogical requested a review from a team as a code owner June 25, 2025 12:57
@aws-cdk-automation aws-cdk-automation requested a review from a team June 25, 2025 12:57
@github-actions github-actions bot added repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK bug This issue is a bug. effort/small Small work item – less than a day of effort p1 labels Jun 25, 2025
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jun 25, 2025
@Abogical Abogical deployed to test-pipeline June 25, 2025 12:58 — with GitHub Actions Active
@aws-cdk-automation
Copy link
Collaborator

➡️ PR build request submitted to test-main-pipeline ⬅️

A maintainer must now check the pipeline and add the pr-linter/cli-integ-tested label once the pipeline succeeds.

@Abogical Abogical force-pushed the fix-dockerignore-nested-exclude branch from 9e88188 to 3913643 Compare June 25, 2025 13:10
@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 25, 2025

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. In particular, if this is a bug fix, please describe the bug that was fixed, not the change you made. (See Contributing Guide, Pull Requests)

@Abogical Abogical changed the title fix(ecr-assets): process nested excludes in .dockerignore correctly fix(ecr-assets): incorrect processing of nested excludes in .dockerignore Jun 25, 2025
@Abogical Abogical changed the title fix(ecr-assets): incorrect processing of nested excludes in .dockerignore fix(ecr-assets): incorrect handling of nested excludes in .dockerignore Jun 25, 2025
rix0rrr
rix0rrr reviewed Jun 25, 2025
View reviewed changes
packages/aws-cdk-lib/core/lib/fs/options.ts Outdated
* @see {@link https://github.com/aws/aws-cdk/issues/13636} for more info on the bug this option fixes.
* @default true if the feature flag DOCKER_IGNORE_NESTED_EXCLUDE_FIX is set to true, otherwise false
*/
readonly copyNestedExcludes?: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would you copy "excludes" ?

Does this mean respectedNestedIgnoreFiles ?

Copy link
Member Author

Choose a reason for hiding this comment

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

"respect" seems to be a better description. I've renamed it to dockerIgnoreRespectNestedExcludes

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a bit verbose. Can you shorten it? For example, do both ignore and exclude not refer to the same thing? So do we need both?

Copy link
Contributor

Choose a reason for hiding this comment

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

respectNestedDockerIgnores?

Copy link
Member Author

@Abogical Abogical Jun 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

"exclude" means something that is excluded from the .dockerignore, so an exclude and ignore are not the same thing, they're the opposite.

**            # Ignore
!build/*.js   # Exclude

Copy link
Member Author

Choose a reason for hiding this comment

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

It is verbose, but I don't think there's a good way to shorten it.

@Abogical Abogical force-pushed the fix-dockerignore-nested-exclude branch from 3913643 to 06574ad Compare June 25, 2025 14:00
@aws-cdk-automation aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 25, 2025
@Abogical Abogical force-pushed the fix-dockerignore-nested-exclude branch from 06574ad to 315d2ff Compare June 25, 2025 16:12
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rix0rrr rix0rrr rix0rrr left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
bug This issue is a bug. contribution/core This is a PR that came from AWS. effort/small Small work item – less than a day of effort p1 pr/needs-maintainer-review This PR needs a review from a Core Team Member repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

(docker assets): .dockerignore is processed incorrectly before 1.73.0 and broken after 1.73.0
3 participants
@Abogical @aws-cdk-automation @rix0rrr