Skip to content

Conversation

carterdanko-dw
Copy link
Contributor

what

  • Initially put the wrong values for coditions, just needs to be a list
  • Bucket should be single resource vs joining on a list.

references

issue #122

@carterdanko-dw carterdanko-dw requested review from a team as code owners April 15, 2025 16:51
@mergify mergify bot added the triage Needs triage label Apr 15, 2025
Copy link
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this PR does anything. join just converts the module.aws_s3_bucket.bucket_arn to a string and if multiple were given, you'd end up with them joined with no separator. With that being the case, these changes were probably never needed to add the join, but now that they're there... I'm unsure if this is providing anything.

Can you please clarify if you were hitting an issue with this code and include the error that you received if so?

Also, mentioning #122 here so it's correctly linked.

@carterdanko-dw
Copy link
Contributor Author

carterdanko-dw commented Apr 16, 2025

@Gowiem Yep!

When trying to apply the stack, I get this error

│ Error: Invalid function argument
│
│   on .terraform/core/modules/cloudtrail_s3_bucket.s3_bucket/sqs_notifications.tf line 36, in data "aws_iam_policy_document" "sqs_policy":
│   36:       values   = [join("", module.aws_s3_bucket.bucket_arn)]
│     ├────────────────
│     │ while calling join(separator, lists...)
│     │ module.aws_s3_bucket.bucket_arn is "arn:aws:s3:::BUCKET_NAME_REDACTED"
│
│ Invalid value for "lists" parameter: list of string required.

after modifying the local .terraform files to test, and re-running my plan I get this error

╷
│ Error: Invalid function argument
│
│   on .terraform/core/modules/cloudtrail_s3_bucket.s3_bucket/sqs_notifications.tf line 49, in resource "aws_s3_bucket_notification" "bucket_notification":
│   49:   bucket = join("", module.aws_s3_bucket.bucket_id)
│     ├────────────────
│     │ while calling join(separator, lists...)
│     │ module.aws_s3_bucket.bucket_id is "BUCKET_NAME_REDACTED"
│
│ Invalid value for "lists" parameter: list of string required.

but then modifying the .terraform file locally and planning was able to create the notification as expected.

FWIW running this with OpenTofu v1.8.3

@carterdanko-dw carterdanko-dw changed the title Wrong valuess for one, and missing the other one. Wrong values for one, and missing the other one. Apr 16, 2025
@carterdanko-dw carterdanko-dw changed the title Wrong values for one, and missing the other one. SNS notification values passed in remediation Apr 16, 2025
@Gowiem Gowiem changed the title SNS notification values passed in remediation fix: remove join calls on bucket arn + id usage Apr 16, 2025
@Gowiem Gowiem added patch A minor, backward compatible change bugfix Change that restores intended behavior and removed triage Needs triage labels Apr 16, 2025
@Gowiem
Copy link
Member

Gowiem commented Apr 16, 2025

/terratest

Copy link
Member

@Gowiem Gowiem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Gowiem Gowiem merged commit ba78d69 into cloudposse:main Apr 16, 2025
26 of 56 checks passed
Copy link
Contributor

These changes were released in v1.4.5.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Change that restores intended behavior patch A minor, backward compatible change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants