feat(s3-deployment): support securityGroups in BucketDeploymentProps#33233
feat(s3-deployment): support securityGroups in BucketDeploymentProps#33233mergify[bot] merged 4 commits intoaws:mainfrom drduhe:issue/33229
Conversation
github-actions
bot
added
beginning-contributor
|
This now has integration tests and documentation for the new feature and the missing tests/documentation for the related VPC feature previously implemented. |
aws-cdk-automation
added
pr-linter/exemption-requested
packages/aws-cdk-lib/aws-s3-deployment/lib/bucket-deployment.ts
Outdated
Show resolved
Hide resolved
github-actions
bot
added
the
effort/medium
|
Any traction on getting this one looked at reviewed? I can't see the build logs as to why it is failing. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #33233 +/- ##
=======================================
Coverage 80.84% 80.84%
=======================================
Files 236 236
Lines 14230 14230
Branches 2487 2487
=======================================
Hits 11504 11504
Misses 2442 2442
Partials 284 284
Flags with carried forward coverage won't be shown. Click here to find out more.
|
The CI is still failing. Looks like this is the start of the failing point |
packages/aws-cdk-lib/aws-s3-deployment/lib/bucket-deployment.ts
Outdated
Show resolved
Hide resolved
aaythapa
added
pr-linter/no-exemption
|
This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. Note that PRs with failing linting check or builds are not reviewed, please ensure your build is passing To prevent automatic closure:
This PR will automatically close in 7 days if no action is taken. |
|
@Mergifyio rebase |
✅ Branch has been successfully rebased |
I think this piece has been resolved |
aws-cdk-automation
dismissed
their stale review
✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.
|
I have added comprehensive integration tests for this new feature as well as the ones that were missing for the VPC feature. The tests are working when I run them locally against my dev account but they seem to be failing when running in the pipeline the package has set up and I can't find much details around the failures in the logs. Could some one step in and help point me in the right direction? |
|
Seems that my latest baseline is up-to-date with the snapshots if you can test them? `(base) 5ce91e835887:aws-cdk drduhe$ yarn integ-runner --directory packages/@aws-cdk --dry-run --update-on-failed Verifying integration test snapshots... UNCHANGED aws-amplify-alpha/test/integ.app-cache-config 3.002s Snapshot Results: Tests: 234 passed, 234 total Running integration tests for failed tests... Running in parallel across regions: us-east-1, us-east-2, us-west-2 Test Results: Tests: 0 passed, 0 total |
|
@Mergifyio update |
✅ Branch has been successfully updated |
|
Looking through the build failures it seems that some of the earlier unrelated tests passed - but something failed to clean up cleanly? I am not sure how to triage that to make an update on my end? As to my failures - can I get specific logs pertaining to why these failed: Edit: Ok - update - the integration tests are failing with the following error ( at least on my end )
It seems these tests can't be run in parallel because the accounts run out of addresses to assign the VPC subnets - please advise, can I make these tests run sequentially - or scope down their testing range? This PR implemented tests for the previously merged VPC feature which is causing this headache - and mine was an attempt to add coverage for that feature per the PR feedback. What I will try next is limiting each of the test deployment VPC configurations to |
|
@drduhe Yes, you can make them run sequentially by using the |
But this will just make it run sequentially in my local dev deployment right? How would we enforce they get run sequentially as part of the production build that happens in the Github pipeline? |
|
@drduhe The Github pipeline currently only checks that the snapshots are matching. It doesn't currently deploy the snapshots automatically. |
|
Ack - running sequentially now |
|
Ok, sorry for the delay but it took like 8+ hours to run all the tests sequentially but they all this passed when I ran it them this final time - I cleaned up the other integration tests the grouping as well with this PR. See results below from my final deployment / tests this evening: |
mergify
bot
dismissed
Abogical’s stale review
Pull request has been modified.
|
Fixed Rosetta README.md errors. Not sure why the |
|
Lemme know if I need to do anything specific - happy to work this down today so it doesn't lose traction again. |
|
Appreciate your efforts on this @drduhe , I want to see this PR done as well. 😅 |
|
@drduhe Please don't force push commits. Its hard to see what changes you made since last review when you do this. |
@Abogical - Ah, I won't do this moving forward, I realize now you support squashing on the merge and I should have left my changes as atomic commits. Pushing another commit now targeting the remaining failures in the Update: It seems to be passing the Rosetta linting workflow now. |
|
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 pull request has been removed from the queue for the following reason: The pull request can't be updated
You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. |
|
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). |
|
Comments on closed issues and PRs are hard for our team to see. |
7 participants
Issue 33229
closes #33229
Reason for this change
The
BucketDeploymentconstruct in AWS CDK allows deploying assets to S3 buckets, often requiring a Lambda function to perform the deployment. Currently, users can specify a custom VPC viaBucketDeploymentProps, ensuring the deployment happens within a restricted network.However, many organizations require more granular network security control. While specifying a VPC is helpful, allowing custom security groups would enable teams to define specific ingress/egress rules, meeting stricter compliance and security requirements.
Description of changes
BucketDeploymentPropsto include an optionalsecurityGroups?: ec2.ISecurityGroup[]property.BucketDeploymentconstructor to passsecurityGroupsto the Lambda function.securityGroupsoptional.vpc,vpcSubnets, andsecurityGroupsparameters.Describe any new or updated permissions being added
N/A
Description of how you validated changes
Added unit tests to the relevant code modules to cover feature usage.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license