[CustomDbEngineVersion] Add new parameters in CEV #478
Conversation
| @@ -27,6 +27,9 @@ public static CreateCustomDbEngineVersionRequest createCustomDbEngineVersionRequ | |||
| .description(model.getDescription()) | |||
| .engine(model.getEngine()) | |||
| .engineVersion(model.getEngineVersion()) | |||
| .sourceCustomDbEngineVersionIdentifier(model.getSourceCustomDBEngineVersionIdentifier()) | |||
| .useAwsProvidedLatestImage(model.getUseAwsProvidedLatestImage()) | |||
| .imageId(model.getImageId()) | |||
There was a problem hiding this comment.
These attributes are returned in the DescribeDBEngine response. You should also include them in the translateFromSdk method in the Translator. This way, CFN will be aware that they have been applied.
| @@ -27,6 +27,9 @@ public static CreateCustomDbEngineVersionRequest createCustomDbEngineVersionRequ | |||
| .description(model.getDescription()) | |||
| .engine(model.getEngine()) | |||
| .engineVersion(model.getEngineVersion()) | |||
| .sourceCustomDbEngineVersionIdentifier(model.getSourceCustomDBEngineVersionIdentifier()) | |||
| .useAwsProvidedLatestImage(model.getUseAwsProvidedLatestImage()) | |||
There was a problem hiding this comment.
If this parameter is not included in the describe response, you should designate it as a write-only parameter in the resource schema. Otherwise, it will consistently result in false drift.
However, if it is returned in the describe response, you should also include it in the translateFromSdk function.
There was a problem hiding this comment.
These are write-only / create-only attributes. I'll add them to lists. Thanks for the reminder!
| @@ -27,6 +27,9 @@ public static CreateCustomDbEngineVersionRequest createCustomDbEngineVersionRequ | |||
| .description(model.getDescription()) | |||
| .engine(model.getEngine()) | |||
| .engineVersion(model.getEngineVersion()) | |||
| .sourceCustomDbEngineVersionIdentifier(model.getSourceCustomDBEngineVersionIdentifier()) | |||
There was a problem hiding this comment.
Can you confirm there no extra permission needed for this operation beside existing permissions.
- "kms:CreateGrant"
- "kms:DescribeKey"
- "mediaimport:CreateDatabaseBinarySnapshot"
- "rds:AddTagsToResource"
- "rds:CreateCustomDBEngineVersion"
- "rds:DeleteCustomDBEngineVersion"
- "rds:DescribeDBEngineVersions"
- "rds:ModifyCustomDBEngineVersion"
- "rds:RemoveTagsFromResource"
- "s3:CreateBucket"
- "s3:GetObject"
- "s3:GetObjectAcl"
- "s3:GetObjectTagging"
- "s3:ListBucket"
- "s3:PutBucketObjectLockConfiguration"
- "s3:PutBucketPolicy"
- "s3:PutBucketVersioning"
There was a problem hiding this comment.
Can you kindly confirm if all possible exceptions have been taken care of?
if the SourceCustomDBEngineVersionIdentifier or ImageId is not found or in an inappropriate state for the operation, what specific exceptions will be thrown?
Moreover, It will need to be associated with a recognized error from CloudFormation by adding it to the CreateHandler error role set, otherwise It will lead to an internal failure on the customer's end.
Yes I checked exceptions that could be thrown from the latest change, all have been covered by existing CreateHandler exceptions:
|
Thanks for the reminder! I do need to add additional permissions. |
|
Superseded by #515 |
Issue #, if available: N / A
Description of changes:
Latest AWS RDS SDK has added three optional parameters for CEV model and originally required field is now no longer compulsory. This PR is to add the additional parameters to CFN accordingly.
Tests:
test_read_input_output_negative_match from contract test doesn't apply for the API, this is because some parameters are not supposed to be used together (e.g. S3 bucket and source CEV). This particular test will fail with below if enabled.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.