-
Notifications
You must be signed in to change notification settings - Fork 97
Conversation
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.
Looking great! I have a few minor changes to request (to use locals).
@@ -22,6 +22,7 @@ module "service-data" { | |||
encrypted = "${var.data_volume_encrypted}" | |||
kms_key_id = "${var.data_volume_kms_key_id}" | |||
snapshot_id = "${var.data_volume_snapshot_id}" | |||
extra_tags = {"createSnapshot" = "${var.name_prefix}"} |
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.
For createSnapshot
, I would recommend the following:
- we add a new
local.data_volume_name_prefix
, set to${var.name_prefix}-${var.name_suffix}-data
and to be used here:
module "service-data" {
source = "../persistent-ebs"
name_prefix = "${var.name_prefix}-${var.name_suffix}-data"
- we add a
local.create_snapshot_tag
, which useslocal.data_volume_name_prefix
- we use that here in
extra_tags
for the SNASG and DLM policy
schedule_create_interval = "${var.dlm_schedule_create_interval}" | ||
schedule_create_time = "${var.dlm_schedule_create_time}" | ||
schedule_retain_rule = "${var.dlm_schedule_retain_rule}" | ||
schedule_tags_to_add = "${merge(map("Name", "${var.name_prefix}-${name-sufix}-dlm", "SnapshotCreator", "DLM lifecycle"))}" |
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.
This would also be a good candidate for separating as a local
value.
|
||
variable "dlm_schedule_copy_tags" { | ||
description = "Copy all user-defined tags on a source volume to snapshots of the volume created by this policy." | ||
default = false |
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.
This ought to default to true
here.
|
||
variable "dlm_ebs_target_tags" { | ||
description = "Tags to filter the volume that we want to take the snapshot." | ||
default = {} |
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.
Is this a required parameter? Or is this an additional filter? Eg, will the module work with this default, and take snapshots, or is this parameter required for the module to take snapshots?
06e1f1f
to
fb2fc63
Compare
I'm going to create a new PR these changes are based on the older version of the module, the current DML policy module is easier to implement. |
This needs a rebase and resolve merge conflicts. |
Sure I'll rebase and make more testing |
This PR is using old DLM code so it's better to close and open a new one, open new PR and run some tests shouldn't take long I think 2-3 hours could be enough. |
Closes #204
- For new
modules
this would entail example code for how to use the module or some explanation in the module readme.