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

fix(ecr-assets): unable to use one Dockerfile to build multiple images #5705

Merged
merged 8 commits into from
Jan 8, 2020
Merged

fix(ecr-assets): unable to use one Dockerfile to build multiple images #5705

merged 8 commits into from
Jan 8, 2020

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Jan 8, 2020

Since we use the source hash as the image ID, if we use different docker build options such as specifying a different target, buildArgs or a custom docker file name, we still get the same image ID. This means that if this image ID already exists, we skip the build. This makes it impossible to use the same docker build context to build multiple images.

This fix exposes the ability to include extra information to the source hash fingerprint algorithm (in assets.Staging), and utilizes this capability to "salt" the hash with build arguments if they are provided.

Fixes #5683

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

fix(ecr-assets): unable to use one Dockerfile to build multiple images
5e6c0fe 
Since we use the source hash as the image ID, if we use different docker build options such as specifying a different `target`, `buildArgs` or a custom docker file name, we still get the same image ID. This means that if this image ID already exists, we skip the build. This makes it impossible to use the same docker build context to build multiple images.

This fix exposes the ability to include extra information to the source hash fingerprint algorithm (in `assets.Staging`), and utilizes this capability to "salt" the hash with build arguments if they are provided.

Fixes #5683
@eladb eladb requested a review from rix0rrr as a code owner January 8, 2020 09:10
@eladb eladb requested a review from iliapolo January 8, 2020 09:10
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Jan 8, 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

RomainMuller
RomainMuller reviewed Jan 8, 2020
View reviewed changes
Copy link
Contributor

@RomainMuller RomainMuller left a comment

Choose a reason for hiding this comment

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

That's a nice feature! I have some minor comments, but mostly stylistic (and quite frankly, besides the name being awkward, it's not that important).

packages/@aws-cdk/assets/test/test.staging.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-ecr-assets/lib/image-asset.ts Outdated Show resolved Hide resolved
iliapolo
iliapolo requested changes Jan 8, 2020
View reviewed changes
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.

@eladb few comments

packages/@aws-cdk/assets/lib/fs/copy-options.ts Show resolved Hide resolved
packages/@aws-cdk/aws-ecr-assets/lib/image-asset.ts Show resolved Hide resolved
packages/@aws-cdk/aws-ecr-assets/lib/image-asset.ts Show resolved Hide resolved
Elad Ben-Israel added 2 commits January 8, 2020 15:32
code review feedback
d4097bd 
- rename `extra` to `extraHash` so it makes more sense when extending `FingerprintOptions`.
- build `extraHash` as an object instead of an array.
- include `repositoryName` in fingerprint
Merge remote-tracking branch 'origin/master' into benisrae/fix-docker…
09bd1db 
…-source-hash-salt
@eladb eladb requested review from iliapolo and RomainMuller January 8, 2020 13:34
@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

@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

@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 Jan 8, 2020

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

mergify bot commented Jan 8, 2020

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

@mergify mergify bot merged commit ff3f27f into master Jan 8, 2020
@mergify mergify bot deleted the benisrae/fix-docker-source-hash-salt branch January 8, 2020 15:56
@amirfireeye
Copy link
Contributor

Can this behavior be disabled or tuned with some flag? We have the opposite problem where we pass CodeArtifact token as a build arg so every single synth generates a new Docker image.

@amirfireeye amirfireeye mentioned this pull request Aug 7, 2021
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iliapolo iliapolo iliapolo approved these changes

@rix0rrr rix0rrr Awaiting requested review from rix0rrr

@RomainMuller RomainMuller Awaiting requested review from RomainMuller

Assignees
No one assigned
Labels
contribution/core This is a PR that came from AWS.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

ecr-assets: unable to use one Dockerfile to build multiple images
5 participants
@eladb @aws-cdk-automation @amirfireeye @RomainMuller @iliapolo