Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FIFO - Period stripped from name variable #40

Closed
klesher opened this issue Oct 21, 2021 · 5 comments
Closed

FIFO - Period stripped from name variable #40

klesher opened this issue Oct 21, 2021 · 5 comments
Labels
bug 🐛 An issue with the system

Comments

@klesher
Copy link

klesher commented Oct 21, 2021

Found a bug? Maybe our Slack Community can help.

Slack Community

Describe the Bug

When specifying a topic name variable ending in .fifo, the period is stripped. This .fifo suffix is required by AWS when naming FIFO queues.

Expected Behavior

.fifo to remain in place as this is required for FIFO queues by AWS.

Steps to Reproduce

Steps to reproduce the behavior:
Create a new queue with name ending in .fifo.

Ex:
Setting it to -fifo leaves it unchanged:
results-test-fifo

But ending in .fifo strips the period:
results-testfifo

Screenshots

If applicable, add screenshots or logs to help explain your problem.

Environment (please complete the following information):

Anything that will help us triage the bug will help. Here are some ideas:
Tested with terraform-aws-sns-topic module versions 19.2 and 17.0 (when this feature was released).

Additional Context

Digging through the module, I see there’s a replace(module.this.id, ".", "-") for display_name, but I’m not seeing why it’s happening for the Topic name.

I was able to work around this by setting the delimiter and attributes values, however this seems like it's not an intended use:

name = "results-test"
delimiter = "."
attributes = ["fifo"]

Results in:
results-test.fifo

@klesher klesher added the bug 🐛 An issue with the system label Oct 21, 2021
@nitrocode
Copy link
Member

nitrocode commented Nov 10, 2021

This is a bug. Good find. The reason the period is stripped is due to regex_replace_chars from the context.tf which defaults to "/[^a-zA-Z0-9-]/" unless overwritten.

For now, you should be able to set this and the period will not be stripped.

  regex_replace_chars = "/[^a-zA-Z0-9-.]/"

@klesher can you try using this branch and see if it works as expected ?

module "sns" {
  source = "git::https://github.com/cloudposse/terraform-aws-sns-topic.git?ref=sqs_queue_allow_periods"
  # ...

  fifo_queue = true
  fifo_topic = true
}

@klesher
Copy link
Author

klesher commented Nov 10, 2021

Hi Nitrocode! Looks like that did the trick! I'm able to remove the delimiter and attributes values I have set now.

The only minor issue (?) I see at this point is that the "Name" tag doesn't have .fifo appended to it. The Name tag is often what AWS uses for ordering in the console, so I try to keep this matching the actual name of a resource where possible. I can work around this if I'm wrong in my assumption, so not a big deal!

  tags = merge({
    terraform-managed = true
    project           = var.project
    environment       = var.environment
    Name              = local.name      <----
  }, var.tags)
  ~ resource "aws_sns_topic" "this" {
      ...
      ~ tags                        = {
          - "Attributes"        = "fifo" -> null
          ~ "Name"              = "results-test.fifo" -> "results-test"
            # (3 unchanged elements hidden)
        }
      ~ tags_all                    = {
          - "Attributes"        = "fifo" -> null
          ~ "Name"              = "results-test.fifo" -> "results-test"
            # (3 unchanged elements hidden)
        }
        # (6 unchanged attributes hidden)
    }

@nitrocode
Copy link
Member

@klesher I updated it and it should be updating the Name tag correctly on the sns topic and queue. Please try it again when you get a chance.

@klesher
Copy link
Author

klesher commented Nov 16, 2021

Hey @nitrocode - just tested this out and it's working as I'd expect!

Thank you for working on this, much appreciated!

@nitrocode
Copy link
Member

@klesher this has been fixed as part of https://github.com/cloudposse/terraform-aws-sns-topic/releases/tag/0.20.0

Thanks again for submitting this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 An issue with the system
Projects
None yet
Development

No branches or pull requests

2 participants