@Igor Rodionov @Andriy Knysh (Cloud Posse)

just another friendly ping on this

@Moritz comment by Andriy:
please add linux_parameters support in a separate PR.
This PR is very old and introduced many changes.
thank you

done in https://github.com/cloudposse/terraform-aws-ecs-web-app/pull/285

I see that the Terratest command performs FULL_VERSION=$(vert -s "$(terraform-config-inspect --json examples/complete | jq -r '.required_core[]')" "$TF012" "$TF013" "$TF014" "$TF015" "$TF1" | head -1) || [[ -n "$VERSION" ]]

However, this repo is kind of a mono-module repo, so instead of examples/complete it has examples/spacelift-policy and examples/spacelift-stack etc.

What are y’all thoughts on this test? Does that seem like the problem in this repo?

Potential options: adding a boilerplate examples/complete or adding a fallback in the test script?

Got the tests fixed with the gracious help from @Veronika Gnilitska! Just pending a review now.

This should also address the other pending PR’s in this same repository where the test fails unrelated to the actual code

@Igor Rodionov @Dan Miller (Cloud Posse)

Merged! Thanks for the contribution!

@jpalomaki looks like there’s some updates on your PR

@Ben Smith (Cloud Posse) @Jeremy White (Cloud Posse)

@Jeremy White (Cloud Posse) bumping this up

Please see comment. It looks like some of the depends_on changes could have caused it:

It’s interesting, I run both terraform and opentofu tests using docker and they always pass - I suppose the opentofu CI job was unlucky? :thinking_face:

I’ve added a depends_on to the aws_s3_bucket_cors_configuration.origin resource in hopes that corrects it. I’ll admit it seems a bit odd of an error, cannot find a resource that it is in the middle of creating.

Looks like latest terratest passed. I believe I corrected the lint formatting with my latest commit and hoping that clears up the bats failure on the next ci run

oof, sorry that I got busy yesterday. Re-running now

sorry looks like i hadn’t saved all the changes before my previous commit just pushed up what i think should handle the failures

not sure what’s up with the chatops action. I’ll look into it on monday if no one else does this weekend

awesome. hope you enjoy the weekend!

@Jeremy White (Cloud Posse) check contributors channel

@Dan Miller (Cloud Posse)

Thanks for submitting the PR! Looks good to me, but I left 1 minor comment

Updated, thanks for checking

@Dan Miller (Cloud Posse)

@nnsense we accidentally introduce a breaking change! I’m fixing it now, but we have to revert this PR https://github.com/cloudposse/terraform-aws-lambda-function/issues/78

OOPS! :smile: Right.. but then.. why name is lowercase? name is basically my reason behind that PR, if I set that to something uppercase, there’s something that set it back to lowercase, which creates a bug for the module, if you set the lambda name to something different from lowercase it will then target a cloudwatch group that doesn’t exist (being all lowercase)

I will try to dig more, this is not the end of it!!

Something that let me think, but I didn’t pay too much attention to it, is hardcoding /aws/lambda/ at the beginning of the cloudwatch group name. That is what I think is actually creating the issue.. Will try to propose a solution

the name is lowercase because the log group name is run through our null-label module, which converts everything to lowercase as a convention https://github.com/cloudposse/terraform-aws-cloudwatch-logs/blob/main/main.tf#L5-L13

I was assuming that but when I check the [variables.tf](http://variables.tf) inside null label it says “default is lower” but then default = null.. I didn’t find where that “lower” is then being set TBH https://github.com/cloudposse/terraform-null-label/blob/e88c5a8e4b8a621d4b96603bfe5c0738b96aec5b/variables.tf#L193

sorry wrong link. looking, one second

that was the old, deprecated module, my fault. Checking

so yes it is label_value_case, which if unset will choose “lower” https://github.com/cloudposse/terraform-null-label/blob/main/main.tf#L14

Got it, thanks :slightly_smiling_face: So.. the problem lies down to the name attribute which is set to be "/aws/lambda/${var.function_name}" Trying to automate avoiding to add an additional variable for whoever wants it camelCase, PascalCase or Title but still trying to avoid breaking who is already relying on this being lower by deefault.. what do you think of…

module "cloudwatch_log_group" {
  source  = "cloudposse/cloudwatch-logs/aws"
  version = "0.6.6"

  enabled = module.this.enabled

  iam_role_enabled  = false
  kms_key_arn       = var.cloudwatch_logs_kms_key_arn
  retention_in_days = var.cloudwatch_logs_retention_in_days
  name              = "/aws/lambda/${var.function_name}"
  label_value_case  = can(regex("[A-Z]", var.function_name)) ? "none" : "lower" <-------------------- this ?

  tags = module.this.tags
}

Or even better null instead of lower since that’s the default

@Dan Miller (Cloud Posse) hi there, any thoughts on the above? I have a number of lambdas all named pascalcase and all have the link with cloud watch broken, would love to fix it :)

so that I follow correctly, is your intention with that label_value_case line to use “none” if there are any upper case characters in the function name and use lower if no?

Yes, it’s weird I know.. But now I’m terrorised do introduce something that’ll break the others work :)

what about other users that have upper case function names?

this will replace their log group, no?

all have the link with cloud watch broken Which is the part that’s broken? Maybe we can fix that instead

That’s my case, and it would be awesome to get it fixed. Like I said in the issue I opened, if my lambda has a name with uppercase letters, the module will create a cloud watch group all lowercase but it will then refer to it as it was originally, with upper case. Easy to test, just deploy a lambda with SomeName, it will reference a group with SomeName but the model will create it somename

Cloudposse powerful null label is usually taking care of this, which is why I originally added the context, but this cloud watch module inside lambda is peculiar because it specifically set name prefixing it with /aws/lambda which I think is the real issue

but it will then refer to it as it was originally, with upper case Forgive me if you already said, but where does your lambda refer to the cloudwatch group? Is that part of the module?

Into the module main.tf

module "cloudwatch_log_group" {
  source  = "cloudposse/cloudwatch-logs/aws"
  version = "0.6.6"

  enabled = module.this.enabled

  iam_role_enabled  = false
  kms_key_arn       = var.cloudwatch_logs_kms_key_arn
  retention_in_days = var.cloudwatch_logs_retention_in_days
  name              = "/aws/lambda/${var.function_name}"
  tags              = module.this.tags
}

name is the cloudposse null-label name into the cloudwatch module which, as you pointed out, is lower by default. This is obviously true for both lambda and cloudwatch module but the lambda module is set by the user context while the cloudwatch module is not. So the cloudwatch group will create the name in lowercase because the context isn’t set by my config and, likely, since no log_group is set into the lambda module to force the name, Lambda will assume its name is the same as the the function itself, prefixed by /aws/lambda. This will set lambda to search for /aws/lambda/MyLogGroup even if the module has created it as /aws/lambda/myloggroup.

The example/complete provided is setting the lambda name with module.label.id which also forces that all lowercase, if I set that to var.function_name, this is the plan:

  # module.lambda.aws_lambda_function.this[0] will be created
  + resource "aws_lambda_function" "this" {
      + architectures                  = (known after apply)
      + arn                            = (known after apply)
      + code_sha256                    = (known after apply)
      + filename                       = "function.zip"
      + function_name                  = "ThisIsMyFunction"    <---------------- PascalCase
      + handler                        = "handler.handler"
      + id                             = (known after apply)
      + invoke_arn                     = (known after apply)
      + last_modified                  = (known after apply)
      + layers                         = []
      + memory_size                    = 128
      + package_type                   = "Zip"
      + publish                        = false
      + qualified_arn                  = (known after apply)
      + qualified_invoke_arn           = (known after apply)
      + reserved_concurrent_executions = -1
      + role                           = (known after apply)
      + runtime                        = "nodejs20.x"
      + signing_job_arn                = (known after apply)
      + signing_profile_version_arn    = (known after apply)
      + skip_destroy                   = false
      + source_code_hash               = (known after apply)
      + source_code_size               = (known after apply)
      + tags_all                       = (known after apply)
      + timeout                        = 3
      + version                        = (known after apply)
    }

  # module.lambda.module.cloudwatch_log_group.aws_cloudwatch_log_group.default[0] will be created
  + resource "aws_cloudwatch_log_group" "default" {
      + arn               = (known after apply)
      + id                = (known after apply)
      + log_group_class   = (known after apply)
      + name              = "/aws/lambda/thisismyfunction" <---------------- lowercase
      + name_prefix       = (known after apply)
      + retention_in_days = 0
      + skip_destroy      = false
      + tags              = {
          + "Name" = "/aws/lambda/thisismyfunction"
        }
      + tags_all          = {
          + "Name" = "/aws/lambda/thisismyfunction"
        }
    }

To sort this out, the best would be to pass context and leave the name up to the user (without /aws/lambda) but I tried and we had to roll the change back because then name ends up getting the context name applied (so, the id). Another solution would be, without my weird “regex” workaround, to set the label_value_case as well into the lambda module’s cloudwatch group, allowing us to set it the same way we set lambda. Unfortunately, this is just my case, if someone else is setting anything else from the root module, it will sill get lost, exactly like label_value_case, but apparently no one else complained so far, so I might be one of the few changing the variables of the root context

Ah well, of course we also have the option to force the log group name into lambda, but I don’t think is fair to enforce a naming convention to the user by unilaterally decide to set everything to lowercase.

I just tested, this works if you think it would be ok, passing the root module setting to cloudwatch, what do you think?

module "cloudwatch_log_group" {
  source  = "cloudposse/cloudwatch-logs/aws"
  version = "0.6.6"

  enabled = module.this.enabled

  iam_role_enabled  = false
  kms_key_arn       = var.cloudwatch_logs_kms_key_arn
  retention_in_days = var.cloudwatch_logs_retention_in_days
  name              = "/aws/lambda/${var.function_name}"
  label_value_case  = var.label_value_case
  tags              = module.this.tags
}

to be honest I’m a little swamped this week with other work, so I may not have much time to look into this at more than a high level. Although I do think adding var.label_value_case as an option is a good compromise

I completely understand and obviously not a problem at all, I will add a PR with the proposed change so that you can review once you have time, thanks :)

do you see the note in description that this module is deprecated? I would recommend you use cloudposse/terraform-aws-dynamic-subnets instead

Ah didnt notice that, thanks.

Wow, I didn’t notice too, and I’m actively using that module. It’s a bit hard to see for me because I usually jump to the readme and read that (and I skip the header, if any :D)

@Dan Miller (Cloud Posse)

Left a comment. We should try to make sure any resource is covered by terratest

Fixed.. well, maybe, not sure if that’s everything needed

@nnsense looks like terraform failed once we enabled these new resources. Can you please review the error? Let me know if you have any questions about it

That’s what I was expecting to happen by adding source_mapping_enabled = true, but I don’t know what to set for source_mapping_arn because that doesn’t exist (should I add a dynamodb example to sort that out?)

I think I’ve sorted that out passing fake parameters, hope that works…

aww right It doesnt

You might need to create an example dynamodb table so that you can properly validate the resource by passing a valid ARN

Let’s do it

Done ( )

@Dan Miller (Cloud Posse)

@Ben Smith (Cloud Posse)

@nnsense left a comment. Same idea as yesterday where we added test coverage for any new resource

Updated, partially.. I need your input to get the bonus points

@Andriy Knysh (Cloud Posse) @Igor Rodionov

@Ray Finch thank you for the PR, it’s merged