Skip to content

feat(core): add network option to docker run command#21450

Merged
mergify[bot] merged 3 commits intoaws:mainfrom
maafk:docker-network-option
Aug 5, 2022
Merged

feat(core): add network option to docker run command#21450
mergify[bot] merged 3 commits intoaws:mainfrom
maafk:docker-network-option

Conversation

@maafk
Copy link
Copy Markdown
Contributor

@maafk maafk commented Aug 3, 2022 •
edited
Loading

closes #21447

Describe the feature

Ability to add --network flag when bundling assets with docker

Use Case

Developers in corporate environments that use locked down developer machines are often restricted in what networks are available to them, and in order to bundle assets properly within a docker container, they must be explicit in the network their container connects to

Proposed Solution

Add optional property to DockerRunOptions and BundlingOptions that would translate into --network {user provided option}

This change would not reflect in any Cloudformation changes, but only with the docker commands performed when bundling

All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional 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
Copy Markdown

gitpod-io bot commented Aug 3, 2022

@github-actions github-actions bot added the p2 label Aug 3, 2022
@aws-cdk-automation aws-cdk-automation requested a review from a team August 3, 2022 17:36
TheRealAmazonKendra
TheRealAmazonKendra previously requested changes Aug 3, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

Please make sure that your PR body describes the problem the PR is solving, and the design approach and alternatives considered. Explain why the PR solves the problem. A link to an issue is helpful, but does not replace an explanation of your thought process (See Contributing Guide, Pull Requests)

PRs that are labeled as a feat also require integ tests and README updates.

@github-actions github-actions bot added the feature-request A feature should be added or improved. label Aug 3, 2022
@maafk
Copy link
Copy Markdown
Contributor Author

maafk commented Aug 3, 2022

Please make sure that your PR body describes the problem the PR is solving, and the design approach and alternatives considered. Explain why the PR solves the problem. A link to an issue is helpful, but does not replace an explanation of your thought process (See Contributing Guide, Pull Requests)

Updated to include issue number and the problem it's solving

PRs that are labeled as a feat also require integ tests and README updates.

This change wouldn't result in any cloudformation changes. It only affects the bundling of assets, so there wouldn't be any change to synthesized cloudformation.

I'm also not seeing any integration tests under the core package that I'd be able to update.

As far as updates to a README I can see a few options, since the core README makes no reference to asset bundling

  1. Make updates in the s3-asset README where it discusses asset bundling
  2. Make updates in the lambda README where it discusses bundling asset code
  3. Add a new section to the core README taking from the first two READMEs and diving deeper into Bundling
  4. Something else I'm not thinking of 😄

Thanks for your help!

@mrgrain
Copy link
Copy Markdown
Contributor

mrgrain commented Aug 4, 2022

@maafk I reckon the s3-asset README is the most appropriate please for this. After all, S3 Asset is the only place where this interface is currently exposed.

The respective section in the lambda README should probably get a deeplink to refer to the S3 Asset docs for more information.

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review August 4, 2022 19:14

Pull request has been modified.

@maafk
Copy link
Copy Markdown
Contributor Author

maafk commented Aug 4, 2022

@maafk I reckon the s3-asset README is the most appropriate please for this. After all, S3 Asset is the only place where this interface is currently exposed.

The respective section in the lambda README should probably get a deeplink to refer to the S3 Asset docs for more information.

@mrgrain I gave it a shot! Happy to incorporate any feedback 😄

@mrgrain
Copy link
Copy Markdown
Contributor

mrgrain commented Aug 5, 2022 •
edited
Loading

Hey @maafk I think your code got a little mixed up here with other branches.

@maafk
Copy link
Copy Markdown
Contributor Author

maafk commented Aug 5, 2022

Hey @maafk I think your code got a little mixed up here with other branches.

I certainly did 🤦 . Just pushed a clean branch

@TheRealAmazonKendra TheRealAmazonKendra changed the title feat(core): Add network option to docker run command feat(core): add network option to docker run command Aug 5, 2022
TheRealAmazonKendra
TheRealAmazonKendra previously requested changes Aug 5, 2022
View reviewed changes
packages/@aws-cdk/aws-lambda/README.md Outdated
});
```

See [Asset Bundling](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_s3_assets-readme.html#asset-bundling) for more details on bundling options
Copy link
Copy Markdown
Contributor

@TheRealAmazonKendra TheRealAmazonKendra Aug 5, 2022

Choose a reason for hiding this comment

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

I don't think this will actually translate correctly across the various ways we publish our documentation. I think we can just remove this here and only include the entry in the s3 assets package.

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review August 5, 2022 15:01

Pull request has been modified.

@aws-cdk-automation
Copy link
Copy Markdown
Collaborator

aws-cdk-automation commented Aug 5, 2022

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 88019da
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

mrgrain
mrgrain approved these changes Aug 5, 2022
View reviewed changes
Copy link
Copy Markdown
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.

Let's do this! :shipit:

@mergify
Copy link
Copy Markdown
Contributor

mergify bot commented Aug 5, 2022

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

@mergify mergify bot merged commit 86e396a into aws:main Aug 5, 2022
@TheRealAmazonKendra
Copy link
Copy Markdown
Contributor

TheRealAmazonKendra commented Aug 5, 2022

Thanks for all your work on this!

@maafk maafk deleted the docker-network-option branch August 5, 2022 17:09
@mrgrain mrgrain added the effort/small Small work item – less than a day of effort label Aug 17, 2022
@patrickmryan patrickmryan mentioned this pull request Aug 31, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mrgrain mrgrain mrgrain approved these changes

@TheRealAmazonKendra TheRealAmazonKendra TheRealAmazonKendra left review comments

Assignees

No one assigned

Labels

effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

(core): Add ability to set docker networking config when bundling assets

4 participants

@maafk @mrgrain @aws-cdk-automation @TheRealAmazonKendra