Change approach to choose instance in order to backup #3

s2504s · 2017-08-17T14:40:53Z

What

Change approach to choose instance for backup.

Why

We do not want to create special tag on each AWS instance in order to backup.

osterman · 2017-08-18T17:10:50Z

ami_backup.py

+    create_fmt = create_time.strftime('%Y-%m-%d')
+
+    AMIid = ec.create_image(InstanceId=ec2_instance_id,
+                            Name="Lambda - " + os.environ['instance_id'] + " from " + create_fmt,


Really don't like this name. Lamda is the strategy, but label_id would be much better and more descriptive. Would that be an easy change?

Also, prefer names to always be in tf_label fmt.

osterman

Use hyphenated Name field: label_id-instance_id-create_fmt

Remove

osterman · 2017-08-18T19:08:50Z

README.md

@@ -24,6 +24,8 @@ module "lambda_ami_backup" {
  namespace         = "${var.namespace}"
  region            = "${var.region}"
  ami_owner         = "${var.ami_owner}"
+  instance_id       = "${var.instance_id}"
+  retention         = "14"


rename to retention_days for clarity

I don't see that it was renamed. It still says retention =P

osterman · 2017-08-18T19:10:59Z

fix README.md repo URL

osterman · 2017-08-18T20:01:51Z

WARNING! AMI cleanup works not yet.

Is this still relevant?

const-bon · 2017-08-22T07:22:36Z

ami_backup.py

-        Filters=[
-            {'Name': 'tag-key', 'Values': ['backup', 'Backup', 'Snapshot']},
+    try:
+        retention_days = os.environ['retention_days']


Let's use explicit transformation retention_days = int(os.environ['retention_days'])

const-bon · 2017-08-22T07:25:29Z

ami_backup.py

-            {'Name': 'tag-key', 'Values': ['backup', 'Backup', 'Snapshot']},
+    try:
+        retention_days = os.environ['retention_days']
+    except:


It is good practice to use named exceptions. In this way we know what exception happened and why we use it.
Let's use except ValueError:

const-bon · 2017-08-22T07:29:35Z

main.tf

@@ -73,6 +73,13 @@ module "label_cleanup" {
  name      = "${var.name}-cleanup"
 }

+module "label_id" {


We don't need label_id module as it is a copy of label module.

Used label module. The module label_id was removed

const-bon · 2017-08-22T07:30:26Z

main.tf

@@ -98,6 +105,9 @@ resource "aws_lambda_function" "ami_backup" {
    variables = {
      region    = "${var.region}"
      ami_owner = "${var.ami_owner}"
+      instance_id = "${var.instance_id}"


Apply terraform fmt

const-bon · 2017-08-22T07:31:59Z

main.tf

@@ -87,7 +94,7 @@ resource "aws_iam_role_policy" "ami_backup" {
 resource "aws_lambda_function" "ami_backup" {
  filename         = "${path.module}/ami_backup.zip"
  function_name    = "${module.label_backup.id}"
-  description      = "Automatically backup instances tagged with 'Snapshot: true'"
+  description      = "Automatically backup EC2 instance using instance id"


Let's use more direct description "Automatically backup EC2 instance ${var.instance_id} (create AMI)"

const-bon · 2017-08-22T07:32:57Z

README.md

@@ -17,13 +17,15 @@ Notes:

 ```
 module "lambda_ami_backup" {
-  source = "git::https://github.com/cloudposse/tf_lambda_ami_backup.git?ref=master"
+  source = "git::https://github.com/cloudposse/tf_ami_backup.git?ref=master"


Pin to the tag which will be added after this PR merged.
I think, we should do major tag version change, because logic of the module was heavily modified.

const-bon · 2017-08-22T07:36:20Z

README.md

@@ -17,13 +17,15 @@ Notes:



Delete

Notes: * `ami_owner` is an AWS account id.

Because there is a Variables section.

osterman · 2017-08-23T06:41:37Z

ami_backup.py

 #
-# After creating the AMI it creates a "DeleteOn" tag on the AMI indicating when
+# After creating the AMI it creates a "ami_delete_on" tag on the AMI indicating when


@comeanother all of our tags are camel case. Not sure why we are changing to snake case.

e.g. Namespace, Stage, Name

Tag has been modified to AMIDeleteOn

const-bon · 2017-08-25T13:55:10Z

Invalidated by #4

Updated README

e290976

s2504s requested review from osterman and const-bon August 17, 2017 14:42

s2504s self-assigned this Aug 17, 2017

s2504s changed the title ~~Updated README~~ Change approach to choose instance in order to backup Aug 17, 2017

Changed ami_backup.py

f4fb620

osterman reviewed Aug 18, 2017

View reviewed changes

osterman requested changes Aug 18, 2017

View reviewed changes

osterman reviewed Aug 18, 2017

View reviewed changes

s2504s force-pushed the without_tags branch from 20964ab to 2fbe32b Compare August 21, 2017 09:27

Changed AMI name

bd20e7b

s2504s force-pushed the without_tags branch from 2fbe32b to bd20e7b Compare August 21, 2017 09:31

Changed AMI name

7689320

osterman previously approved these changes Aug 21, 2017

View reviewed changes

const-bon suggested changes Aug 22, 2017

View reviewed changes

s2504s dismissed osterman’s stale review via b2e91f5 August 22, 2017 11:20

Changed ami_backup.py main.tf README.md

9ce31a7

s2504s force-pushed the without_tags branch from b2e91f5 to 9ce31a7 Compare August 22, 2017 11:24

osterman reviewed Aug 23, 2017

View reviewed changes

Modified the tag

e523249

osterman approved these changes Aug 24, 2017

View reviewed changes

const-bon closed this Aug 25, 2017

const-bon deleted the without_tags branch August 25, 2017 13:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change approach to choose instance in order to backup #3

Change approach to choose instance in order to backup #3

s2504s commented Aug 17, 2017 •

edited

Loading

osterman Aug 18, 2017

osterman Aug 18, 2017

s2504s Aug 21, 2017

osterman left a comment

osterman Aug 18, 2017

s2504s Aug 21, 2017

osterman Aug 21, 2017

osterman commented Aug 18, 2017 •

edited by s2504s

Loading

osterman commented Aug 18, 2017

const-bon Aug 22, 2017

const-bon Aug 22, 2017

const-bon Aug 22, 2017

s2504s Aug 22, 2017

const-bon Aug 22, 2017

s2504s Aug 22, 2017

const-bon Aug 22, 2017

const-bon Aug 22, 2017

s2504s Aug 22, 2017

const-bon Aug 22, 2017

s2504s Aug 22, 2017

osterman Aug 23, 2017

osterman Aug 23, 2017

s2504s Aug 23, 2017

const-bon commented Aug 25, 2017

Change approach to choose instance in order to backup #3

Change approach to choose instance in order to backup #3

Conversation

s2504s commented Aug 17, 2017 • edited Loading

What

Why

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osterman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osterman commented Aug 18, 2017 • edited by s2504s Loading

osterman commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

const-bon commented Aug 25, 2017

s2504s commented Aug 17, 2017 •

edited

Loading

osterman commented Aug 18, 2017 •

edited by s2504s

Loading