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.

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

feat(ecr-assets): Support .dockerignore (faster Docker builds) #4104

Merged
merged 12 commits into from
Sep 22, 2019
Merged

feat(ecr-assets): Support .dockerignore (faster Docker builds) #4104

merged 12 commits into from
Sep 22, 2019

Conversation

parisholley
Copy link
Contributor

If your Dockerfile lives in the project root, CDK will copy ALL files (node_modules, etc) into the cdk out folder prior to running the docker build command (which is super slow). Docker has support for a .dockerignore file that ignores files/directories when building the context:

https://docs.docker.com/engine/reference/builder/#dockerignore-file

@mergify
Copy link
Contributor

mergify bot commented Sep 16, 2019

Thanks so much for taking the time to contribute to the AWS CDK ❤️

We will shortly assign someone to review this pull request and help get it
merged. In the meantime, please take a minute to make sure you follow this
checklist:

  • PR title type(scope): text
    • type: fix, feat, refactor go into CHANGELOG, chore is hidden
    • scope: name of module without aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
    • text: use all lower-case, do not end with a period, do not include issue refs
  • PR Description
    • Rationale: describe rationale of change and approach taken
    • Issues: indicate issues fixed via: fixes #xxx or closes #xxx
    • Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>
  • Testing
    • Unit test added. Prefer to add a new test rather than modify existing tests
    • CLI or init templates change? Re-run/add CLI integration tests
  • Documentation
    • README: update module README to describe new features
    • API docs: public APIs must be documented. Copy from official AWS docs when possible
    • Design: for significant features, follow design process

@parisholley parisholley changed the title Support .dockerignore (faster Docker builds) feat(ecr-assets): Support .dockerignore (faster Docker builds) Sep 16, 2019
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • 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: FAILED
  • Build Logs (available for 30 days)

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

eladb
eladb previously requested changes Sep 17, 2019
View reviewed changes
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Please follow the PR checklist and let us know when this is ready for a review.

@mergify mergify bot dismissed eladb’s stale review September 19, 2019 17:41

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • Result: FAILED
  • 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

@parisholley
Copy link
Contributor Author

@rix0rrr @eladb g2g

@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 previously requested changes Sep 20, 2019
View reviewed changes
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

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

Please revert all unnecessary changes and add an explicit test to verify .dockerignore behavior

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
@mergify mergify bot dismissed eladb’s stale review September 20, 2019 14:08

Pull request has been modified.

@parisholley
Copy link
Contributor Author

@eladb done, sounds like we need some lint updates :)

@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

@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

@mergify
Copy link
Contributor

mergify bot commented Sep 22, 2019

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

@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

@mergify mergify bot merged commit 8389eeb into aws:master Sep 22, 2019
eladb pushed a commit that referenced this pull request Sep 23, 2019
@parisholley
feat(ecr-assets): Support .dockerignore (faster Docker builds) (#4104)
ad351f8 
* Support .dockerignore (faster Docker builds)

* ts fix

* test update and lint fix

* exclude dockerignore so it does not influence hashing

* tweaks

* remove verify from other test

* revert

* more revert
@kolomied
Copy link
Contributor

kolomied commented Oct 9, 2019 •
edited
Loading

It seems we do not handle common "exclude everything except" pattern for .dockerignore files (Docker docs):

# Ignore everything
*
# Allow files and directories
!/src/**

With the current implementation the first 'star' glob effectively excludes everything from the asset folder.

@rix0rrr @eladb - guys, should I create an issue?

@kolomied kolomied mentioned this pull request Oct 10, 2019
Closed
@iliapolo iliapolo mentioned this pull request Jan 20, 2020
Closed
@anguslees anguslees mentioned this pull request Mar 24, 2020
Closed
@yurigorokhov
Copy link

bump, regarding not supporting pattern @kolomied brought up.

@kolomied
Copy link
Contributor

@yurigorokhov I believe the problem was addressed in #4450

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

@eladb eladb eladb approved these changes

@rix0rrr rix0rrr Awaiting requested review from rix0rrr

Assignees

@eladb eladb

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@parisholley @aws-cdk-automation @kolomied @yurigorokhov @eladb