feat(runner-binaries-syncer): add s3_tags variable for additional S3 bucket tagging #4832
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for additional S3 bucket tagging in the runner-binaries-syncer module. Users can now specify custom tags for S3 buckets at both the module level and individual runner configuration level.
- Introduces
runner_binaries_s3_tagsvariable for module-level S3 bucket tagging - Adds
runner_binaries_s3_tagsfield to multi-runner configuration for runner-specific tagging - Implements tag merging logic that combines module-level and runner-specific tags
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| variables.tf | Adds module-level runner_binaries_s3_tags variable |
| modules/runner-binaries-syncer/variables.tf | Adds s3_tags variable to syncer module |
| modules/runner-binaries-syncer/main.tf | Updates S3 bucket resource to merge default and additional tags |
| modules/multi-runner/variables.tf | Adds runner_binaries_s3_tags to runner config and module variables |
| modules/multi-runner/runner-binaries.tf | Passes merged tags to runner binaries module |
| modules/multi-runner/main.tf | Implements tag merging logic for different OS/architecture combinations |
| main.tf | Passes S3 tags variable to runner binaries module |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
@npalm could I get your input on this? |
|
Sorry won't be able to review any PR till the end of the month. |
|
@damianrekosz can you check the commen from copilot. Can you also quickly explain the use case for different tags on the bucket. |
The comment from Copilot isn't accurate. Removing
Yeah, sure. Some organizations may tag their S3 buckets differently - eg each bucket might be tagged with its name or department to provide better cost management visibility when custom tags in cost allocation are enabled. |
There was a problem hiding this comment.
@damianrekosz thx for the PR, Sorry for the waiting time. PR looks in general ok. Made some suggestion comments on the multi runner. No need create a complex local object. Simply pass the tags down the mdule multi-runner module.
| tmp_distinct_list_unique_os_and_arch = distinct([for i, config in local.runner_config : { "os_type" : config.runner_config.runner_os, "architecture" : config.runner_config.runner_architecture } if config.runner_config.enable_runner_binaries_syncer]) | ||
| unique_os_and_arch = { for i, v in local.tmp_distinct_list_unique_os_and_arch : "${v.os_type}_${v.architecture}" => v } | ||
|
|
||
| s3_tags = { |
There was a problem hiding this comment.
I think this local for s3_tags is not needed. The multi runner crates a bucket per github dist. Which means not a bucket er multi runner. So would simple pass the s3_tags down to the module. And no cmplext merge.
| } | ||
| } | ||
|
|
||
| variable "runner_binaries_s3_tags" { |
There was a problem hiding this comment.
This variable is ok, lets remove the one on runner config lvel.
| state_event_rule_binaries_syncer = var.state_event_rule_binaries_syncer | ||
|
|
||
| server_side_encryption_configuration = var.runner_binaries_s3_sse_configuration | ||
| s3_tags = local.s3_tags[each.key] |
There was a problem hiding this comment.
| s3_tags = local.s3_tags[each.key] | |
| s3_tags = var.runner_binaries_s3_tags |
2 participants
This pull request introduces an option to provide additional tags for S3 buckets created by the
binaries-syncermodule.S3 tags can be specified using the
runner_binaries_s3_tagsvariable at the module level to apply tags to all S3 buckets at once, or at the individual runner configuration level to define different tags for specific runners. Tags defined at both the module and runner configuration levels are merged when their OS and architecture match.