nimblehq / infrastructure-templates

For IaaS and PaaS as codes
MIT License
10 stars 0 forks source link

[#220] Fix: namespace and environment are misused #222

Closed malparty closed 11 months ago

malparty commented 11 months ago

What happened ๐Ÿ‘€

Insight ๐Ÿ“

I'm not sure about how could we add tests for this change, feel free to suggest anything ๐Ÿ™Œ

Proof Of Work ๐Ÿ“น

image

Nihisil commented 11 months ago

I like this clean up ๐Ÿงน

The question is why we need namespace at all? I found only one usage at the moment: "name": "${namespace}",, and probably this usage was missed during refactoring and it should be "name": "${env_namespace}", in service.json.tfpl.

malparty commented 11 months ago

Nice catch @Nihisil ๐Ÿ™

a89a771 โœ…

As for the question do we need it at all?, I agree we could remove it:

      locals {
        namespace = "app-name"
        env_namespace = "${namespace}-${var.environment}"
      }`;
      locals {
        env_namespace = "app-name-${var.environment}"
      }`;

I've done it in 3aee9bd but feel free to let me know which way you prefer (it's quick to rollback that small change ๐Ÿ˜‡). Thanks ๐Ÿ™Œ

Nihisil commented 11 months ago

Updated way looks great for me, I have only one remained thing. It looks like our security group is broken, it tries to read env namespace from local, but it should be var. It applies only to /modules/security_group/main.tf file and all other places looks good.

image
malparty commented 11 months ago

Nice catch!! ๐Ÿคฏ Thanks, I've found that multiple security groups were actually declared in this file, this 107494d should fix them all ๐Ÿ‘

Nihisil commented 11 months ago

Great work ๐ŸŽ‰ , but I brought you something new (pls don't hate me):

$ terraform validate
โ•ท
โ”‚ Error: Invalid function argument
โ”‚
โ”‚   on ../modules/ecs/main.tf line 35, in locals:
โ”‚   35:   container_definitions = templatefile("${path.module}/service.json.tftpl", local.container_vars)
โ”‚     โ”œโ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€
โ”‚     โ”‚ while calling templatefile(path, vars)
โ”‚     โ”‚ local.container_vars is object with 13 attributes
โ”‚
โ”‚ Invalid value for "vars" parameter: vars map does not contain key "namespace", referenced at
โ”‚ ../modules/ecs/service.json.tftpl:19,37-46.
hoangmirs commented 11 months ago

Great work ๐ŸŽ‰ , but I brought you something new (pls don't hate me):

$ terraform validate
โ•ท
โ”‚ Error: Invalid function argument
โ”‚
โ”‚   on ../modules/ecs/main.tf line 35, in locals:
โ”‚   35:   container_definitions = templatefile("${path.module}/service.json.tftpl", local.container_vars)
โ”‚     โ”œโ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€โ”€
โ”‚     โ”‚ while calling templatefile(path, vars)
โ”‚     โ”‚ local.container_vars is object with 13 attributes
โ”‚
โ”‚ Invalid value for "vars" parameter: vars map does not contain key "namespace", referenced at
โ”‚ ../modules/ecs/service.json.tftpl:19,37-46.

@malparty This might be the last place that we need to change ๐Ÿ‘‰ https://github.com/nimblehq/infrastructure-templates/blob/develop/templates/addons/aws/modules/ecs/service.json.tftpl#L19