Breaks or not while handling Lambda filter,mapper errors

[acharp]

This change is

[germangh]

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.

---

humilis_push_processor/lambda_function/handler/init.py, line 113 at r1 (raw file):

    if "{{sns_topic_name}}" == "None":
        logger.error("No SNS Topic name provided, layer push-processor "
                     "needs one to report the errors.")

This should be a CriticalError. Otherwise the error will only show up in the logs, which nobody ever looks at without a notification first.

---

humilis_push_processor/lambda_function/handler/init.py, line 115 at r1 (raw file):

                     "needs one to report the errors.")
    else:
        sns = boto3.client("sns")

Instead of having sns_topic_name as the parameter for the layer I would have sns_topic_arn. Then you avoid calls to boto3, and you can also simplify a bit the CF resources file. Also, since the SNS topic is not part of this application but something set up externally it makes more sense to use the ARN. What if the user wanted to send the notification to an SNS topic in another AWS region? Or in another AWS account?

---

Comments from Reviewable

[acharp]

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.

---

humilis_push_processor/lambda_function/handler/init.py, line 113 at r1 (raw file):

Previously, germangh (German Gomez-Herrero) wrote…

This should be a CriticalError. Otherwise the error will only show up in the logs, which nobody ever looks at without a notification first.

Done.

---

humilis_push_processor/lambda_function/handler/init.py, line 115 at r1 (raw file):

Previously, germangh (German Gomez-Herrero) wrote…

Instead of having sns_topic_name as the parameter for the layer I would have sns_topic_arn. Then you avoid calls to boto3, and you can also simplify a bit the CF resources file. Also, since the SNS topic is not part of this application but something set up externally it makes more sense to use the ARN. What if the user wanted to send the notification to an SNS topic in another AWS region? Or in another AWS account?

I agree, it makes more sense passing the ARN.

---

Comments from Reviewable