Skip to content
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

Merged
merged 4 commits into from
Jan 29, 2020

Conversation

nathanpeck
Copy link
Member

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:

You can even use the .dockerignore file to exclude the Dockerfile and .dockerignore files. These files are still sent to the daemon because it needs them to do its job. But the ADD and COPY instructions do not copy them to the image.

(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 project


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

@nathanpeck nathanpeck requested a review from eladb January 28, 2020 23:36
@mergify
Copy link
Contributor

mergify bot commented Jan 28, 2020

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@nathanpeck nathanpeck changed the title Don't leave Dockerfile or .dockerignore out or the assets folder if in the .dockerignore file fix: Don't leave Dockerfile or .dockerignore out or the assets folder if in the .dockerignore file Jan 28, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@eladb eladb requested a review from iliapolo January 29, 2020 02:46
@nathanpeck nathanpeck changed the title fix: Don't leave Dockerfile or .dockerignore out or the assets folder if in the .dockerignore file fix: Don't leave Dockerfile or .dockerignore out of the assets folder if in the .dockerignore file Jan 29, 2020
Copy link
Contributor

@iliapolo iliapolo left a 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).

packages/@aws-cdk/aws-ecr-assets/lib/image-asset.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecr-assets/lib/image-asset.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecr-assets/lib/image-asset.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecr-assets/package.json Show resolved Hide resolved
@piradeepk piradeepk changed the title fix: Don't leave Dockerfile or .dockerignore out of the assets folder if in the .dockerignore file fix(ecr-assets): Don't leave Dockerfile or .dockerignore out of the assets folder if in the .dockerignore file Jan 29, 2020
@mergify mergify bot dismissed iliapolo’s stale review January 29, 2020 16:09

Pull request has been modified.

@nathanpeck
Copy link
Member Author

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 .dockerignore files for the existing tests in order to trigger the right conditions for the bug to manifest, verified that the tests broke as expected, added new code to fix the bug, and then verified that the tests went back to passing.

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-cdk-automation
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@iliapolo
Copy link
Contributor

@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 .dockerignore includes an entry that ignores the docker file.

For example: #5507

This is because this title is then shown in the release notes, and it helps customers clearly
understand what was fixed.

Sorry for not mentioning this before, it just dawned on me.

@nathanpeck nathanpeck changed the title fix(ecr-assets): Don't leave Dockerfile or .dockerignore out of the assets folder if in the .dockerignore file fix(ecr-assets): docker images are not built if .dockerignore includes an entry that ignores the dockerfile. Jan 29, 2020
@mergify
Copy link
Contributor

mergify bot commented Jan 29, 2020

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

@mergify mergify bot merged commit e7ef5e5 into aws:master Jan 29, 2020
@nathanpeck nathanpeck deleted the peckn/dockerfile branch January 29, 2020 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker image assets do not copy the Dockerfile if Dockerfile is included in the .dockerignore file
4 participants