Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(rds): add support for update and backup properties to Cluster instances #10324

Merged
merged 8 commits into from
Sep 24, 2020
Merged

feat(rds): add support for update and backup properties to Cluster instances #10324

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2938686
feat(rds): support several simple parameters for DatabaseCluster
hixi-hyi Sep 12, 2020
b271725
Merge branch 'master' into hixi-hyi/rds-supports-several-simple-param…
hixi-hyi Sep 13, 2020
42443ea
PR feedback (coding style)
hixi-hyi Sep 23, 2020
dc8c509
PD feedback(unnecesarry space & default value)
hixi-hyi Sep 24, 2020
61a2890
Changing to a test that is not the default value
hixi-hyi Sep 24, 2020
6d39dda
Merge branch 'master' into hixi-hyi/rds-supports-several-simple-param…
mergify[bot] Sep 24, 2020
b6d511d
Adjust test after the rename of `masterUser` to `credentials`
skinny85 Sep 24, 2020
291c222
Missed one test
skinny85 Sep 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions packages/@aws-cdk/aws-rds/lib/cluster.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,9 @@ function createInstances(cluster: DatabaseClusterNew, props: DatabaseClusterBase
dbParameterGroupName: instanceParameterGroupConfig?.parameterGroupName,
monitoringInterval: props.monitoringInterval && props.monitoringInterval.toSeconds(),
monitoringRoleArn: monitoringRole && monitoringRole.roleArn,
autoMinorVersionUpgrade: props.instanceProps.autoMinorVersionUpgrade,
allowMajorVersionUpgrade: props.instanceProps.allowMajorVersionUpgrade,
deleteAutomatedBackups: props.instanceProps.deleteAutomatedBackups,
});

// If removalPolicy isn't explicitly set,
Expand Down
19 changes: 19 additions & 0 deletions packages/@aws-cdk/aws-rds/lib/props.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,25 @@ export interface InstanceProps {
* @default - default master key
*/
readonly performanceInsightEncryptionKey?: kms.IKey;

/**
* Whether to enable automatic upgrade of minor version for the DB instance.
*
* @default - true
*/
skinny85 marked this conversation as resolved.
Show resolved Hide resolved
readonly autoMinorVersionUpgrade?: boolean;
/**
hixi-hyi marked this conversation as resolved.
Show resolved Hide resolved
* Whether to allow upgrade of major version for the DB instance.
*
* @default - true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this really true by default? The documentation says:

Constraints: Major version upgrades must be allowed when specifying a value for the EngineVersion parameter that is a different major version than the DB instance's current version.

I guess that suggests that the default is, in fact, false...?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely the default value for AllowMajorVersionUpgrade may be false.
Reading the documentation, this parameter may not be used routinely and is only needed when changing instance types, but I don't understand exactly what it is. (There was no value for AllowMajorVersionUpgrade in the results of aws rds describe-instances)

The @default value is currently written from the Cloudformation documentation.
However, I haven't programmatically set it, and its behavior is dependent on Cloudformation.

Should such a parameter be written as @default None? Or should I explicitly set true or false in the cdk?

I'd like to hear your opinion.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If not setting it and setting it to false provide the same result (as I believe it does in this case), it should be @default false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understood. Thank you for your reply.
In fact, AllowMajorUpgrade is default false, and it seems to be necessary to add this parameter when upgrading the major version.

*/
readonly allowMajorVersionUpgrade?: boolean;
/**
hixi-hyi marked this conversation as resolved.
Show resolved Hide resolved
* Whether to remove automated backups immediately after the DB instance is deleted for the DB instance.
*
* @default - true
*/
readonly deleteAutomatedBackups?: boolean;
}

/**
Expand Down
75 changes: 75 additions & 0 deletions packages/@aws-cdk/aws-rds/test/test.cluster.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -373,6 +373,81 @@ export = {
},
},

'cluster with disable automatic upgrade of minor version'(test: Test) {
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');

// WHEN
new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.AURORA,
masterUser: {
username: 'admin',
},
instanceProps: {
instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL),
autoMinorVersionUpgrade: false,
vpc,
},
});

expect(stack).to(haveResource('AWS::RDS::DBInstance', {
AutoMinorVersionUpgrade: false,
}));

test.done();
},

'cluster with disallow upgrade of major version'(test: Test) {
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');

// WHEN
new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.AURORA,
masterUser: {
username: 'admin',
},
instanceProps: {
instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL),
allowMajorVersionUpgrade: false,
vpc,
},
});

expect(stack).to(haveResource('AWS::RDS::DBInstance', {
AllowMajorVersionUpgrade: false,
}));

test.done();
},

'cluster with disallow remove backups'(test: Test) {
// GIVEN
const stack = testStack();
const vpc = new ec2.Vpc(stack, 'VPC');

// WHEN
new DatabaseCluster(stack, 'Database', {
engine: DatabaseClusterEngine.AURORA,
masterUser: {
username: 'admin',
},
instanceProps: {
instanceType: ec2.InstanceType.of(ec2.InstanceClass.BURSTABLE2, ec2.InstanceSize.SMALL),
deleteAutomatedBackups: false,
vpc,
},
});

expect(stack).to(haveResource('AWS::RDS::DBInstance', {
DeleteAutomatedBackups: false,
}));

test.done();
},

'create a cluster using a specific version of MySQL'(test: Test) {
// GIVEN
const stack = testStack();
Expand Down