-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
fix(ecr-assets): docker images are not built if .dockerignore includes an entry that ignores the dockerfile. #6007
Conversation
…n the .dockerignore file
Title does not follow the guidelines of Conventional Commits. Please adjust title before merge. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
One more thing :)
Please include the relevant module in the PR title: fix(esr-assets)
.
Pull request has been modified.
PR is now updated with support for custom Dockerfile names. Per @iliapolo feedback I have retained the approach of filtering the list of excludes, reduced the explanatory comment verbosity, and reused some existing constants. Tests were actually already existing on this package, but the tested sample was just missing the right conditions to trigger this bug. I added relevant lines to the If wanted I can still break this out into its own entirely new separate test case for clarity, but I figured for the sake of conciseness it made sense to just use the existing test cases. It appears those test cases are already being overloaded to do multiple different types of tests for correctness, so it didn't look like this project has a strict 1 to 1 policy for test condition to test case |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@nathanpeck Looks great. Just a small phrasing issue... We want the PR title to describe the bug itself, and not the solution. That is: docker images are not built if For example: #5507 This is because this title is then shown in the release notes, and it helps customers clearly Sorry for not mentioning this before, it just dawned on me. |
Thank you for contributing! Your pull request is now being automatically merged. |
This fixes #6004 by ensuring that
Dockerfile
and.dockerignore
will always be added to the asset staging folder, even if included in the .dockerignore file. This obeys the spec according to Docker:(https://docs.docker.com/engine/reference/builder/)
Note: this PR adds new dependency
minimatch
to the@aws-cdk/aws-ecr-asset
package, in order to do file glob matching. However this package is also used in@aws-cdk/assets
so this is not a new dependency for the overall projectBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license