fix: sg arn output

[dmitrijn]

what

• When you're using external sg with setting create_security_group = false and don't want to create default one which is included in module you get response with nullable element and can't concatenate null values.

│ Error: Invalid function argument
│ 
│   on .terraform/modules/elasticache_memcached/outputs.tf line 12, in output "security_group_arn":
│   12:   value       = join("", module.aws_security_group[*].arn)
│     ├────────────────
│     │ while calling join(separator, lists...)
│     │ module.aws_security_group is object with 4 attributes
│ 
│ Invalid value for "lists" parameter: element 0 is null; cannot concatenate null values.

[z0rc]

This should fix a long standing issue #45.

[jamengual]

can you please run:

make precommit/terraform

[aknysh]

@dmitrijn thanks for the PR, please see comments

[aknysh]

@dmitrijn ping :)

[dmitrijn]

@aknysh sorry for late answer, fixed! please run tests 🙂

[dmitrijn]

are you planning to merge this PR?

[dmitrijn]

@aknysh ping 🙂

[dmitrijn]

@aknysh @jamengual ping

[jamengual]

/terratest

[dmitrijn]

@jamengual can you merge?

[jamengual]

can you please run:

make precommit/terraform

@dmitrijn

[adamwshero]

This very issue is handled nicely in the redis module so I'm frustrated as to why this isn't merged so we can actually use this module instead of forking it and fixing it ourselves. https://github.com/cloudposse/terraform-aws-elasticache-redis/blob/main/main.tf#L152

Please merge? Pretty please??

@aknysh @jamengual @dmitrijn ping :-)

[Gowiem]

@adamwshero thanks for the bump. We get a lot of PRs + issues with the hundreds of modules, so it's typically a "squeaky wheels gets the oil" type of situation. If you're ever annoyed by this, feel free to ping in the #pr-reviews channel in the SweetOps slack and we're pretty responsive there.

Sadly, this PR is a bit out of date and our CI has changed which has caused it to get into a base state (or at least it looks that way to me). @dmitrijn would you mind pulling in main into your branch (or rebasing) and pushing those changes? That should restart the process and we'll hopefully be able to get this merged once our CI goes green. Thanks!

[adamwshero]

squeak squeak @dmitrijn @Gowiem

[adamwshero]

bump

[Gowiem]

@adamwshero mind forking / copying @dmitrijn's change and putting up a PR yourself that is forked from main? We can get this merged quickly, we just need CI checks to not be in the way and that means a new branch or rebase off main. Add me as a reviewer and I'll work to get it shipped 👍

[pcn]

@adamwshero mind forking / copying @dmitrijn's change and putting up a PR yourself that is forked from main? We can get this merged quickly, we just need CI checks to not be in the way and that means a new branch or rebase off main. Add me as a reviewer and I'll work to get it shipped 👍

Not @adamwshero , but I have this problem so I opened #67 since it's such a small change. Looking forward to seeing it get merged!

[Gowiem]

@adamwshero @dmitrijn this was fixed in #67 and has been released in https://github.com/cloudposse/terraform-aws-elasticache-memcached/releases/tag/0.19.1 -- Sorry for the delay and that we didn't get it done as part of this PR, but at least it's fixed! Thanks for your work here -- closing out 👍