-
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(neptune): auto minor version upgrade for an instance #31988
Changes from 3 commits
41f2f41
f60fb66
4ae5309
cb24723
2a74d37
c38728e
cc0685e
b02de5a
49593db
47d4770
f2f1b7d
d9d7928
57d5385
ca79757
774374c
3e90ca7
036241e
a1cf1d0
126fb2a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -403,6 +403,13 @@ export interface DatabaseInstanceProps { | |
* @default RemovalPolicy.Retain | ||
*/ | ||
readonly removalPolicy?: cdk.RemovalPolicy; | ||
|
||
/** | ||
* Indicates that minor version patches are applied automatically. | ||
* | ||
* @default - same as the cluster's autoMinorVersionUpgrade setting | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the default in CDK is undefined. Do not refer to the default value in the service. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, does this mean that it is preferable to specify There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The DESIGN_GUIDELINES says that it is better to avoid don't write @default undefined, describe the behavior that happens if the property is not supplied. https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md#defaults Therefore, I think it would be better to describe how the current behavior works. What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. but IMO, we should not mention the CFN default behaviour, since we will not update this value if CFN decided to change that default value. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've updated my description to awslint.json {
"exclude": [
"props-physical-name:@aws-cdk/aws-neptune-alpha.ClusterParameterGroupProps",
"props-physical-name:@aws-cdk/aws-neptune-alpha.ParameterGroupProps",
"props-physical-name:@aws-cdk/aws-neptune-alpha.SubnetGroupProps",
"docs-public-apis:@aws-cdk/aws-neptune-alpha.ServerlessScalingConfiguration",
"props-no-undefined-default:@aws-cdk/aws-neptune-alpha.DatabaseInstanceProps.autoMinorVersionUpgrade" // added
]
} |
||
*/ | ||
readonly autoMinorVersionUpgrade?: boolean; | ||
} | ||
|
||
/** | ||
|
@@ -494,6 +501,7 @@ export class DatabaseInstance extends DatabaseInstanceBase implements IDatabaseI | |
super(scope, id); | ||
|
||
const instance = new CfnDBInstance(this, 'Resource', { | ||
autoMinorVersionUpgrade: props.autoMinorVersionUpgrade, | ||
dbClusterIdentifier: props.cluster.clusterIdentifier, | ||
dbInstanceClass: props.instanceType._instanceType, | ||
availabilityZone: props.availabilityZone, | ||
|
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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 created an integration test with a mixed
autoMinorVersionUpgrade
setting for verification, and it is deploying successfully.