feat: Add support for SQS #11
Conversation
aws/sqs/main.tf
Outdated
| # This helps avoid queue names ending in "-" or "-.fifo" | ||
| given_queue_name = var.queue_name == "" ? "" : "-${var.queue_name}" | ||
| # All fifo queues must end in .fifo, per AWS rules | ||
| queue_suffix = var.is_fifo == true ? ".fifo" : "" |
There was a problem hiding this comment.
var.is_fifo ? ".fifo" : ""
There was a problem hiding this comment.
Oh, we have the == is true in a couple of other places, I think? I thought I just needed it. I'll fix that.
There was a problem hiding this comment.
If you find any others, feel free to lump in a cleanup of 'em 😁
aws/sqs/main.tf
Outdated
| policy = jsonencode( | ||
| { | ||
| Version : "2008-10-17" | ||
| Id : "__default_policy_ID" |
There was a problem hiding this comment.
this line seems unnecessary
aws/sqs/main.tf
Outdated
| Id : "__default_policy_ID" | ||
| Statement : [ | ||
| { | ||
| Sid : "__owner_statement" |
There was a problem hiding this comment.
this line also seems unnecessary
aws/sqs/main.tf
Outdated
| visibility_timeout_seconds = var.visibility_timeout_seconds | ||
| } | ||
|
|
||
| resource "aws_sqs_queue_policy" "this" { |
There was a problem hiding this comment.
On a more general note, we probably shouldn't have a default policy, especially one so permissive (this one makes any action on the queue open to anyone in the world i believe). At best, we could have a policy statement variable that can be passed in. For most cases, permission to the queue should be granted through an IAM role rather than a queue policy
There was a problem hiding this comment.
Good call. I started this a month ago based on what we had in perfected. I had to make a number of minor tweaks, so I'm not surprised the policy isn't perfect. I think cognito does a policy passing, or something close to that. i'll review and see if I can follow the same thing.
aws/sqs/outputs.tf
Outdated
| value = aws_sqs_queue.this.arn | ||
| } | ||
|
|
||
| output "full_queue_name" { |
There was a problem hiding this comment.
nit, i think calling it name is good enough
There was a problem hiding this comment.
Fair. I thought it was a bit weird that you'd have something like:
module "sqs" {
queue_name = "blah"
}and then later:
queue_name = module.sqs.nameand those not being the same thing.
aws/sqs/terraform.tf
Outdated
| @@ -0,0 +1,3 @@ | |||
| terraform { | |||
| experiments = [module_variable_optional_attrs] | |||
| } | |||
There was a problem hiding this comment.
this file doesn't seem necessary (I don't see any use of the optional type
There was a problem hiding this comment.
I was using defaults earlier. I'll remove that.
aws/sqs/variables.tf
Outdated
| type = string | ||
| } | ||
|
|
||
| variable "queue_name" { |
There was a problem hiding this comment.
nit, i've been calling these identifier to be less ambiguous, though your description does do a decent job clarifying, so.. 🤷♂️
aws/sqs/variables.tf
Outdated
| variable "receive_wait_time_seconds" { | ||
| description = "The time to wait when polling for new messages. Use 0 for immediate response. Longer values are preferred. AWS recommends a maximum of 20 seconds." | ||
| type = number | ||
| default = 5 |
There was a problem hiding this comment.
I shortened it because 20 is the maximum, and I think the configuration we were looking at in perfected was 10 seconds. Thinking about this a bit more, and doing a bit more reading, I think it makes sense to set it to a large value, and have those that need to adjust downward.
aws/sqs/main.tf
Outdated
| resource "aws_sqs_queue" "this" { | ||
| name = local.full_queue_name | ||
| fifo_queue = var.is_fifo | ||
| content_based_deduplication = var.content_based_deduplication |
There was a problem hiding this comment.
Maybe we should enforce that this is false for non-fifo queues (with a note in the variable description)?
content_based_deduplication = var.is_fifo && var.content_based_deduplication
There was a problem hiding this comment.
Ah, I was trying to do something like that, while still allowing for a fifo: true / dedupe: false, and I was having problems doing that. && should do it though.
This PR adds support for SQS deployments. This tries to be opinionated in the following ways:
This also provides the generated name of the queue as an output, which will typically be needed by any deployment.