Initial attempt at new RDS blueprint framework #86
Conversation
|
|
||
| ENGINE = None | ||
|
|
||
| def _get_engine_versions(self): |
There was a problem hiding this comment.
if something should be overridden, i usually don't put an underscore since it implies some sort of public api
| class ReadReplica(MasterInstance): | ||
| """ Blueprint for a Read replica RDS Database Instance. """ | ||
| def _get_parameters(self): | ||
| parameters = common_parameters() |
There was a problem hiding this comment.
why not just do:
parameters = super(ReadReplica, self).get_parameters()
and have common_parameters returned
There was a problem hiding this comment.
There are parameters that come from the MasterInstance that shouldn't be used in ReadReplicas. A better design might be to have a BaseInstance type or something, and have MasterInstance & ReadReplica both subclass off of it, rather than ReadReplica being based on MasterInstance.
There was a problem hiding this comment.
i see yea, not a big deal, you're just calling common_parameters and overriding it in both places.
| t.add_resource( | ||
| DBInstance( | ||
| DBINSTANCE_NO_IOPS, | ||
| Condition="NoProvisionedIOPS", |
There was a problem hiding this comment.
i would pull out all of these common parameters so i don't have to duplicate specifying them in each
There was a problem hiding this comment.
Hard/ugly to do? How would you do it? Just args/*kwargs?
There was a problem hiding this comment.
i was just thinking like:
common = {
'AllocatedStorage': Ref("AllocatedStorage"),
'AllowMajorVersionUpgrade': Ref("AllowMajorVersionUpgrade"),
'AutoMinorVersionUpgrade': Ref("AutoMinorVersionUpgrade"),
'BackupRetentionPeriod': Ref("BackupRetentionPeriod"),
'DBName': Ref("DatabaseName"),
'DBInstanceClass': Ref("InstanceType"),
'DBSubnetGroupName': Ref(SUBNET_GROUP),
'Engine': self.ENGINE or Ref("Engine"),
'EngineVersion': Ref("EngineVersion"),
'MasterUsername': Ref("MasterUser"),
'MasterUserPassword': Ref("MasterUserPassword"),
'MultiAZ': Ref("MultiAZ"),
'PreferredBackupWindow': Ref("PreferredBackupWindow"),
'VPCSecurityGroups': [Ref(SECURITY_GROUP), ],
'StorageEncrypted': Ref("StorageEncrypted"),
}
t.add_resource(
DBInstance(
DBINSTANCE_NO_IOPS,
Condition="NoProvisionedIOPS",
**common
)
)
t.add_resource(
DBInstance(
DBINSTANCE_WITH_IOPS,
Condition="ProvisionedIOPS",
StorageEncrypted=Ref("StorageEncrypted"),
StorageType="io1",
Iops=Ref("IOPS"),
**common
)
)which also has the added benefit of letting me see whats different about the two.
not sure if thats 100% accurate just a rough copy paste
|
+1 i like it! |
This tries to pull in every DBInstance Parameter in as a 'default'. The
only ones not modified are:
AvailabilityZone: Can't be used w/ MultiAZ, can be added later or in
subclasses
CharacterSetName: Oracle only
DBInstanceIdentifier: Very dependent on the underlying Engine
DBSecurityGroups: Not used in VPC
DBSnapshotIdentifier: Used to create database from backup, could be done
later
Port: Default dependent on Engine, don't want to mess with it.
PubliclyAccessible: Kind of hard to use, not the primary use case really
Mappings evidently only allow alphanumeric values in keys, which breaks things. This isn't as useful without it, and it's a bit more parameters to setup, but at least it works. Also cleans up the need for multiple DBInstances per stack by using AWS:NoValue.
|
+1 again :) |
- create base class for RDS - use mixins for each of the engine specific classes - Built in logic to figure out major version allowed from version - allow subclasses to set their own allowed values for: - engine_major_version - db family - Got rid of ENGINE class variable, use engine() method now - added example confs & envs for mysql & postgres clusters
Initial attempt at new RDS blueprint framework
Still a WIP - mostly opening for discussion about the new
framework.