-
Notifications
You must be signed in to change notification settings - Fork 82
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
Add bucket versioning #1522
Add bucket versioning #1522
Conversation
Explanation of Checkov Baseline Updates:
Testing:
|
Overall changes look good! Thanks for the PR @noah-paige. I have 2 general comments:
I wanted to verify that the update works for existing deployments, so I am merging this branch to an existing environment to double test the PR. I'll post here the results ---> UPDATE: all good! |
@dlpzx - added artifacts bucket that is versioned for data.all pipelines as well and updated the checkov baseline. Tests passing that are synthesizing template with the new manually created bucket. I think we are okay with the replication bucket - given that versioning is helpful for back up and restore information as needed, the replication bucket is only used for FE to replicate info already stored elsewhere and its source of truth is upstream from a different bucket. I think for now there is no risk allowing CDK to create that bucket with its own defaults Going to re-request your review - please give one more look at PR when you have time |
f'{artifact_bucket_base_name}-bucket', | ||
bucket_name=f'{artifact_bucket_base_name}-bucket', | ||
block_public_access=s3.BlockPublicAccess.BLOCK_ALL, | ||
removal_policy=RemovalPolicy.RETAIN, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Do we really want removal_policy=RemovalPolicy.RETAIN,
? When we delete a pipeline stack I think the artefacts bucket is no longer used. wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agreed - updated
Feature or Bugfix
Detail
Relates
N/A
Security
Please answer the questions below briefly where applicable, or write
N/A
. Based onOWASP 10.
fetching data from storage outside the application (e.g. a database, an S3 bucket)?
eval
or similar functions are used?By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.