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(ecs): ContainerImage.fromDockerImageAsset #6093

Merged
merged 5 commits into from
Feb 5, 2020
Merged

feat(ecs): ContainerImage.fromDockerImageAsset #6093

merged 5 commits into from
Feb 5, 2020

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Feb 4, 2020

Allow using an existing DockerImageAsset object as a container image in order to enable direct access to DockerImageAssets API such as accessing the ECR repository, the source hash or granting permissions.

The reason this could not have been exposed through the normal fromImageAsset is that ContainerImage can be used multiple times (i.e. be bound to multiple container definitions), so there is no reliable way to allow users to access the asset.

Related to #5791 and #5983

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

feat(ecs): ContainerImage.fromDockerImageAsset
b94577a 
Allow using an existing `DockerImageAsset` object as a container image in order to enable direct access to `DockerImageAsset`s API such as accessing the ECR repository, the source hash or granting permissions.

The reason this could not have been exposed through the normal `fromImageAsset` is that `ContainerImage` can be used multiple times (i.e. be bound to multiple container definitions), so there is no reliable way to allow users to access the asset.

Related to #5791 and #5983
@github-actions
Copy link

github-actions bot commented Feb 4, 2020

🚫 Features must contain a change to a README file

@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Feb 4, 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

@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

@piradeepk piradeepk added the pr/do-not-merge This PR should not be merged at this time. label Feb 4, 2020
piradeepk
piradeepk approved these changes Feb 4, 2020
View reviewed changes
Copy link
Contributor

@piradeepk piradeepk left a comment

Choose a reason for hiding this comment

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

Had one minor comment. Added the do not merge tag, and approved it as everything else lgtm. Feel free to remove the tag

packages/@aws-cdk/aws-ecr-assets/lib/image-asset.ts
readonly directory: string;

/**
* Options for DockerImageAsset
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we can be more descriptive as to when users should use DockerImageAssetOptions vs DockerImageAssetProps? (we've had users who have been confused with ContainerDefinitionOptions vs ContainerDefinitionProps in the past)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure there is something super smart to write here... Maybe something like "includes all the optional configuration" (without the mandatory one)...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll update in a subsequent commit. Let me know what you think.

@eladb eladb added pr/do-not-merge This PR should not be merged at this time. and removed pr/do-not-merge This PR should not be merged at this time. labels Feb 5, 2020
@mergify
Copy link
Contributor

mergify bot commented Feb 5, 2020

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

@eladb eladb removed the pr/do-not-merge This PR should not be merged at this time. label Feb 5, 2020
@eladb eladb merged commit 38e9865 into master Feb 5, 2020
@eladb eladb deleted the benisrae/docker-assets branch February 5, 2020 13:31
@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

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

@piradeepk piradeepk piradeepk approved these changes

@iliapolo iliapolo Awaiting requested review from iliapolo

Assignees

@piradeepk piradeepk

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.
3 participants
@eladb @aws-cdk-automation @piradeepk