-
Notifications
You must be signed in to change notification settings - Fork 4k
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(appconfig): environment deletion protection #32737
base: main
Are you sure you want to change the base?
feat(appconfig): environment deletion protection #32737
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32737 +/- ##
=======================================
Coverage 66.96% 66.96%
=======================================
Files 329 329
Lines 18667 18667
Branches 3260 3260
=======================================
Hits 12501 12501
Misses 5839 5839
Partials 327 327
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
Thank you for the contribution.
I've left some comments.
I'm not very familiar with this property, so please correct me if I'm wrong.
|
||
You can enable [deletion protection](https://docs.aws.amazon.com/appconfig/latest/userguide/deletion-protection.html) on the environment by setting the `deletionProtection` property. | ||
|
||
- ACCOUNT_DEFAULT: The default setting, which instructs AWS AppConfig to implement the deletion protection value specified in the UpdateAccountSettings API. |
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.
Very very nit:
- ACCOUNT_DEFAULT: The default setting, which instructs AWS AppConfig to implement the deletion protection value specified in the UpdateAccountSettings API. | |
- ACCOUNT_DEFAULT: The default setting, which instructs AWS AppConfig to implement the deletion protection value specified in the UpdateAccountSettings API. |
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.
I understand this sentence means the following, is that correct?
- Use account-level settings when ACCOUNT_DEFAULT is set.
- If users want to enable account-level deletion protection, they need to use the
UpdateAccountSettings
API via AWS CLI or another tool.
The current explanation seems to be taken from the docs, but I think an explanation like the one below would be clearer. What do you think?
The default setting, which uses account-level deletion protection. To configure account-level deletion protection, use the UpdateAccountSettings API.
|
||
- ACCOUNT_DEFAULT: The default setting, which instructs AWS AppConfig to implement the deletion protection value specified in the UpdateAccountSettings API. | ||
- APPLY: Instructs the deletion protection check to run, even if deletion protection is disabled at the account level. APPLY also forces the deletion protection check to run against resources created in the past hour, which are normally excluded from deletion protection checks. | ||
- BYPASS: Instructs AWS AppConfig to bypass the deletion protection check and delete a configuration profile even if deletion protection would have otherwise prevented it. |
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.
Just to clarify, if deletion protection is applied to an environment, is the protected resource the environment itself or a configuration set?
I'm not sure if "configuration profile" is the correct term in this context.
@@ -141,6 +141,32 @@ abstract class EnvironmentBase extends Resource implements IEnvironment, IExtens | |||
} | |||
} | |||
|
|||
/** |
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.
In my understanding, this enum is required for the configuration profile.
Therefore, I think it would be better to separate this enum into another file (e.g., util.ts
).
@mazyu36 Thank you very much for your review! All of your comments are spot-on and provide appropriate feedback. I've addressed all the discussions. |
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.
Thank you for the modification.
Just two more comments.
Co-authored-by: Matsuda <[email protected]>
Co-authored-by: Matsuda <[email protected]>
@mazyu36 Thank you for your reveiw! And I'm sorry for my rough edits. |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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.
Thanks! LGTM
Issue # (if applicable)
None
Reason for this change
AWS AppConfig environment supports deletion protection and this feature is not configurable from AWS CDK.
Description of changes
DeletionProtectionCheck
enumdeletionProtectionCheck
prop toEnvironmentOption
There are two entities,
EnvironmentOptions
andEnvironmentProps
, whereEnvironmentProps
is designed as an extension ofEnvironmentOptions
with the addition of anapplication
prop.Therefore, the current argument addition has also been made to
EnvironmentOptions
.Describe any new or updated permissions being added
None
Description of how you validated changes
Add both unit and integ test.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license