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

feat(core): allow asset bundling on docker remote host / docker in docker #23576

Merged
merged 41 commits into from Jan 27, 2023

Conversation

webratz
Copy link
Contributor

@webratz webratz commented Jan 5, 2023

Fixes #8799

This implements an alternative variant on how to put get files into bundling containers. This is more flexible in its use cases for complex Docker setup scenarios but more complex and slower. Therefore it is not enabled as a default, but as an additional option.

For details to the approach please refer to the linked issue.


All Submissions:

Adding new Construct Runtime Dependencies:

  • This PR adds new construct runtime dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • 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

@gitpod-io
Copy link

gitpod-io bot commented Jan 5, 2023

@github-actions github-actions bot added repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 labels Jan 5, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team January 5, 2023 15:41
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

@aws-cdk-automation aws-cdk-automation dismissed their stale review January 5, 2023 15:54

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

@mrgrain
Copy link
Contributor

mrgrain commented Jan 5, 2023

This is great! What I'm currently wondering is if this should be a prop, feature flag or ENV variable. 🤔

Having it as a prop feels odd in the sense that it's very environment dependent. But then we do the same with local bundling already. @rix0rrr do you have a view how these kind of options should be enabled/disabled?

@webratz
Copy link
Contributor Author

webratz commented Jan 13, 2023

did you get to a decision on how enabling of this mode should be implemented?

@mrgrain
Copy link
Contributor

mrgrain commented Jan 14, 2023

Still on it sorry. Had a busy week.

@mrgrain mrgrain self-assigned this Jan 14, 2023
@mrgrain
Copy link
Contributor

mrgrain commented Jan 16, 2023

Okay I've arrived at the conclusion that a prop is the best option here. It's the only way to enable other uses as well, so if a team wants to implement this based on an environment variable they could do something like:

{
   fileCopyVariant: process.env.BUNDLING_FILE_COPY_VARIANT 
}

Similar applies to using a Context value.

Still need to think about naming, reviewing and manually testing this locally.


Apologies in advance but this PR will likely take me a couple more weeks. Just so you know in case your planning depends on it.

@webratz
Copy link
Contributor Author

webratz commented Jan 16, 2023

Sure, no problem. Feel also free to edit the branch / fork it again or so, if that helps you to speed up things. The overall important thing is the outcome in the end: A more flexible way that allows to use docker bundling in more situations than today.

Copy link
Contributor

@mrgrain mrgrain left a comment

Choose a reason for hiding this comment

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

I'm going back and fourth on the naming of things. Will need to sleep over it.

You can get started on the other changes, but mostly if you could make the build pass (rosetta documentation error), I can use this branch to do a full build and run some proper tests using Docker in Docker

packages/@aws-cdk/aws-lambda-go/README.md Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-lambda-go/test/bundling.test.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/bundling.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/bundling.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/bundling.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/bundling.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/bundling.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/bundling.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/core/lib/asset-staging.ts Outdated Show resolved Hide resolved
@webratz
Copy link
Contributor Author

webratz commented Jan 25, 2023

updated names everywhere

webratz and others added 3 commits January 25, 2023 16:57
Co-authored-by: Momo Kornher <mail@moritzkornher.de>
Co-authored-by: Momo Kornher <mail@moritzkornher.de>
Co-authored-by: Momo Kornher <mail@moritzkornher.de>
@mrgrain mrgrain added pr/do-not-merge This PR should not be merged at this time. and removed pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested labels Jan 26, 2023
mrgrain
mrgrain previously approved these changes Jan 26, 2023
@mergify mergify bot dismissed mrgrain’s stale review January 26, 2023 18:00

Pull request has been modified.

@mrgrain mrgrain added the pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested label Jan 27, 2023
@mrgrain mrgrain removed the pr/do-not-merge This PR should not be merged at this time. label Jan 27, 2023
@mergify
Copy link
Contributor

mergify bot commented Jan 27, 2023

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

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: c4b09ee
  • 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 afce30a into aws:main Jan 27, 2023
@mergify
Copy link
Contributor

mergify bot commented Jan 27, 2023

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p1 pr-linter/cli-integ-tested Assert that any CLI changes have been integ tested repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[core] allow asset bundling on docker remote host / docker in docker
3 participants