feat(ecr-assets): Support docker outputs flag #23304
Conversation
|
|
github-actions
bot
added
p2
beginning-contributor
github-actions
bot
added
effort/small
|
I think this should be ready for a review now? Happy to make any changes needed though! |
|
Hi, this has been open for a few weeks (over the holidays I know) - I was just wondering if anyone might be able to provide feedback or help clear up the ci warning. |
|
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week. |
|
Hello, is there anything I can do to make sure this PR doesnt get cleaned up by the bot as Im about to hit 4 weeks? |
|
Hey @mrgrain - sorry to bug you with a ping. I saw you've worked on some similar PRs on this repo, and I was hoping to get some feedback before the bot closes this PR. I'm sure you're pretty busy getting past the holidays, I just don't want to lose momentum here. |
|
All good @jesse-peters . I'm not back from annual leave yet, but assigned the PR to me. If it gets auto closed before I get to it, just reopen a new one and tag me . |
|
Sounds great, thanks! |
There was a problem hiding this comment.
Code looks good. Just some very minor fixes. I also need to run the CLI tests then which might take a couple days for me to get to.
More generally speaking - would there be a way to detect if buildx is running and default the ouput to --output=type=docker in that case? Or maybe we really should have an image asset that explicitly uses buildx. 🤔
| /** | ||
| * Outputs | ||
| * | ||
| * @default - No outputs |
There was a problem hiding this comment.
Let's be a bit more descriptive here. It's also okay to link to the docker docs with @see
There was a problem hiding this comment.
@jesse-peters Once you've update this doc block and re-generated the schema, the PR should be good to go.
There was a problem hiding this comment.
@mrgrain I believe I pushed this last night?
There was a problem hiding this comment.
Yep I saw that. This is a different place that you've probably just missed. Unfortunately there's a lot of repetition with this. =/
There was a problem hiding this comment.
Oops, you're right! I just pushed an update.
packages/@aws-cdk/cloud-assembly-schema/lib/assets/docker-image-asset.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/cloud-assembly-schema/lib/cloud-assembly/metadata-schema.ts
Outdated
Show resolved
Hide resolved
packages/@aws-cdk/cloud-assembly-schema/schema/assets.schema.json
Outdated
Show resolved
Hide resolved
…e-asset.ts Co-authored-by: Momo Kornher <mail@moritzkornher.de>
…adata-schema.ts Co-authored-by: Momo Kornher <mail@moritzkornher.de>
Co-authored-by: Momo Kornher <mail@moritzkornher.de>
|
I think if we were to default it to I think an image asset that's built for buildx would probably be the best in the long run. This flag exists on standard docker as well, but there are a number of new ones that are buildx specific. |
Interesting idea. I like it. It might require a lot of duplicated constructs and flags in various places. 🤔 |
|
Yea, there probably would be a fair bit of duplication between the two. I think I hit the issues you pointed out on this PR. But I'll keep mulling over a new asset/how we might modify the current one to play nicer with |
mrgrain
added
the
pr-linter/cli-integ-tested
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
|
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
This adds the `--output` flag as an option when building docker containers. This fixes aws#20566. ---- ### All Submissions: * [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) ### Adding new Construct Runtime Dependencies: * [ ] This PR adds new construct runtime dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-construct-runtime-dependencies) ### New Features * [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)? * [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)? *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
This adds the
--outputflag as an option when building docker containers. This fixes #20566.All Submissions:
Adding new Construct Runtime Dependencies:
New Features
yarn integto deploy the infrastructure and generate the snapshot (i.e.yarn integwithout--dry-run)?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license