-
Notifications
You must be signed in to change notification settings - Fork 166
feat: Set kms_key_identifier for EventBridge archives #175
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
feat: Set kms_key_identifier for EventBridge archives #175
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a few minor requests (such as resolving CI failures) before the merge.
Co-authored-by: Anton Babenko <[email protected]>
7cba568
to
b91a2a3
Compare
main.tf
Outdated
|
||
description = lookup(each.value, "description", null) | ||
desired_state = lookup(each.value, "desired_state", null) | ||
kms_key_identifier = var.kms_key_identifier |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be using lookup and fetch from each like the rest of arguments? I think it should in all places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the kms key is specified in the module variables, not per pipe. I think it depends a bit how you would want that to be implemented. The current approach would mean, that pipes, archives and eventbus use the same key specified on the module variables. If you wanted to use different keys, you can set up individual modules for archive, pipes and eventbus, though. That's what I do currently, as well.
Does that work for you? Or do I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User may want to use different keys for different resources, so using lookup for each is a common pattern. Please update it, and example if necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, please review.
It is ok for now, I will update/fix examples later (in a couple of weeks)... |
72fa89e
into
terraform-aws-modules:master
## [4.1.0](v4.0.0...v4.1.0) (2025-07-09) ### Features * Set kms_key_identifier for EventBridge archives ([#175](#175)) ([72fa89e](72fa89e))
This PR is included in version 4.1.0 🎉 |
Thanks for your work, mate! |
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
Description
The Terraform AWS provider introduces the kms_key_identifier property for the "aws_cloudwatch_event_archive" resource. This is not yet set by the terraform-aws-eventbridge module
Motivation and Context
This enables users to set a custom kms key
Breaking Changes
No, not directly. Although I'm not yet sure how an existing archive reacts to re-keying. Might be worth mentioning.
How Has This Been Tested?
examples/*
to demonstrate and validate my change(s)examples/*
projectspre-commit run -a
on my pull request