Skip to content

Changes instance_types from a Set to a List, so instance order preference is preserved #1154

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Sep 30, 2021
Merged

Changes instance_types from a Set to a List, so instance order preference is preserved #1154

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5b0c4be
Preserve ordering of chosen instance types
Makeshift Sep 2, 2021
e273d2e
Switch from for_each to count due to for_each not supporting lists fo…
Makeshift Sep 2, 2021
b3e1981
Missed a each.value
Makeshift Sep 2, 2021
9c06c0c
Format launch template list in the order we specify
Makeshift Sep 2, 2021
cc5467c
typo
Makeshift Sep 2, 2021
65d0227
Missed a change from set to list
Makeshift Sep 2, 2021
81ea37a
Remove unneeded change
Makeshift Sep 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 3 additions & 18 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,23 +343,6 @@ No requirements.
| aws | n/a |
| random | n/a |

## Modules

| Name | Source | Version |
|------|--------|---------|
| runner_binaries | ./modules/runner-binaries-syncer | |
| runners | ./modules/runners | |
| ssm | ./modules/ssm | |
| webhook | ./modules/webhook | |

## Resources

| Name |
|------|
| [aws_resourcegroups_group](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/resourcegroups_group) |
| [aws_sqs_queue](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/sqs_queue) |
| [random_string](https://registry.terraform.io/providers/hashicorp/random/latest/docs/resources/string) |

## Inputs

| Name | Description | Type | Default | Required |
Expand All @@ -380,7 +363,7 @@ No requirements.
| idle\_config | List of time period that can be defined as cron expression to keep a minimum amount of runners active instead of scaling down to 0. By defining this list you can ensure that in time periods that match the cron expression within 5 seconds a runner is kept idle. | <pre>list(object({<br> cron = string<br> timeZone = string<br> idleCount = number<br> }))</pre> | `[]` | no |
| instance\_profile\_path | The path that will be added to the instance\_profile, if not set the environment name will be used. | `string` | `null` | no |
| instance\_type | [DEPRECATED] See instance\_types. | `string` | `"m5.large"` | no |
| instance\_types | List of instance types for the action runner. | `set(string)` | `null` | no |
| instance\_types | List of instance types for the action runner. | `list(string)` | `null` | no |
| key\_name | Key pair name | `string` | `null` | no |
| kms\_key\_arn | Optional CMK Key ARN to be used for Parameter Store. This key must be in the current account. | `string` | `null` | no |
| lambda\_s3\_bucket | S3 bucket from which to specify lambda functions. This is an alternative to providing local files directly. | `any` | `null` | no |
Expand All @@ -398,6 +381,7 @@ No requirements.
| runner\_binaries\_syncer\_lambda\_timeout | Time out of the binaries sync lambda in seconds. | `number` | `300` | no |
| runner\_binaries\_syncer\_lambda\_zip | File location of the binaries sync lambda zip file. | `string` | `null` | no |
| runner\_boot\_time\_in\_minutes | The minimum time for an EC2 runner to boot and register as a runner. | `number` | `5` | no |
| runner\_egress\_rules | List of egress rules for the GitHub runner instances. | <pre>list(object({<br> cidr_blocks = list(string)<br> ipv6_cidr_blocks = list(string)<br> prefix_list_ids = list(string)<br> from_port = number<br> protocol = string<br> security_groups = list(string)<br> self = bool<br> to_port = number<br> description = string<br> }))</pre> | <pre>[<br> {<br> "cidr_blocks": [<br> "0.0.0.0/0"<br> ],<br> "description": null,<br> "from_port": 0,<br> "ipv6_cidr_blocks": [<br> "::/0"<br> ],<br> "prefix_list_ids": null,<br> "protocol": "-1",<br> "security_groups": null,<br> "self": null,<br> "to_port": 0<br> }<br>]</pre> | no |
| runner\_extra\_labels | Extra labels for the runners (GitHub). Separate each label by a comma | `string` | `""` | no |
| runner\_group\_name | Name of the runner group. | `string` | `"Default"` | no |
| runner\_iam\_role\_managed\_policy\_arns | Attach AWS or customer-managed IAM policies (by ARN) to the runner IAM role | `list(string)` | `[]` | no |
Expand Down Expand Up @@ -431,6 +415,7 @@ No requirements.
| runners | n/a |
| ssm\_parameters | n/a |
| webhook | n/a |

<!-- END OF PRE-COMMIT-TERRAFORM DOCS HOOK -->

## Contribution
Expand Down
29 changes: 3 additions & 26 deletions modules/runners/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,31 +58,6 @@ No requirements.
|------|---------|
| aws | n/a |

## Modules

No Modules.

## Resources

| Name |
|------|
| [aws_ami](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/ami) |
| [aws_caller_identity](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/caller_identity) |
| [aws_cloudwatch_event_rule](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cloudwatch_event_rule) |
| [aws_cloudwatch_event_target](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cloudwatch_event_target) |
| [aws_cloudwatch_log_group](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/cloudwatch_log_group) |
| [aws_iam_instance_profile](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_instance_profile) |
| [aws_iam_policy_document](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/data-sources/iam_policy_document) |
| [aws_iam_role](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role) |
| [aws_iam_role_policy](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy) |
| [aws_iam_role_policy_attachment](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/iam_role_policy_attachment) |
| [aws_lambda_event_source_mapping](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/lambda_event_source_mapping) |
| [aws_lambda_function](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/lambda_function) |
| [aws_lambda_permission](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/lambda_permission) |
| [aws_launch_template](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/launch_template) |
| [aws_security_group](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/security_group) |
| [aws_ssm_parameter](https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/ssm_parameter) |

## Inputs

| Name | Description | Type | Default | Required |
Expand All @@ -93,6 +68,7 @@ No Modules.
| block\_device\_mappings | The EC2 instance block device configuration. Takes the following keys: `device_name`, `delete_on_termination`, `volume_type`, `volume_size`, `encrypted`, `iops` | `map(string)` | `{}` | no |
| cloudwatch\_config | (optional) Replaces the module default cloudwatch log config. See https://docs.aws.amazon.com/AmazonCloudWatch/latest/monitoring/CloudWatch-Agent-Configuration-File-Details.html for details. | `string` | `null` | no |
| create\_service\_linked\_role\_spot | (optional) create the service linked role for spot instances that is required by the scale-up lambda. | `bool` | `false` | no |
| egress\_rules | List of egress rules for the GitHub runner instances. | <pre>list(object({<br> cidr_blocks = list(string)<br> ipv6_cidr_blocks = list(string)<br> prefix_list_ids = list(string)<br> from_port = number<br> protocol = string<br> security_groups = list(string)<br> self = bool<br> to_port = number<br> description = string<br> }))</pre> | <pre>[<br> {<br> "cidr_blocks": [<br> "0.0.0.0/0"<br> ],<br> "description": null,<br> "from_port": 0,<br> "ipv6_cidr_blocks": [<br> "::/0"<br> ],<br> "prefix_list_ids": null,<br> "protocol": "-1",<br> "security_groups": null,<br> "self": null,<br> "to_port": 0<br> }<br>]</pre> | no |
| enable\_cloudwatch\_agent | Enabling the cloudwatch agent on the ec2 runner instances, the runner contains default config. Configuration can be overridden via `cloudwatch_config`. | `bool` | `true` | no |
| enable\_organization\_runners | n/a | `bool` | n/a | yes |
| enable\_ssm\_on\_runners | Enable to allow access to the runner instances for debugging purposes via SSM. Note that this adds additional permissions to the runner instances. | `bool` | n/a | yes |
Expand All @@ -102,7 +78,7 @@ No Modules.
| idle\_config | List of time period that can be defined as cron expression to keep a minimum amount of runners active instead of scaling down to 0. By defining this list you can ensure that in time periods that match the cron expression within 5 seconds a runner is kept idle. | <pre>list(object({<br> cron = string<br> timeZone = string<br> idleCount = number<br> }))</pre> | `[]` | no |
| instance\_profile\_path | The path that will be added to the instance\_profile, if not set the environment name will be used. | `string` | `null` | no |
| instance\_type | [DEPRECATED] See instance\_types. | `string` | `"m5.large"` | no |
| instance\_types | List of instance types for the action runner. | `set(string)` | `null` | no |
| instance\_types | List of instance types for the action runner. | `list(string)` | `null` | no |
| key\_name | Key pair name | `string` | `null` | no |
| kms\_key\_arn | Optional CMK Key ARN to be used for Parameter Store. | `string` | `null` | no |
| lambda\_s3\_bucket | S3 bucket from which to specify lambda functions. This is an alternative to providing local files directly. | `any` | `null` | no |
Expand Down Expand Up @@ -150,6 +126,7 @@ No Modules.
| role\_runner | n/a |
| role\_scale\_down | n/a |
| role\_scale\_up | n/a |

<!-- END OF PRE-COMMIT-TERRAFORM DOCS HOOK -->

## Philips Forest
Expand Down
8 changes: 4 additions & 4 deletions modules/runners/main.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ locals {
userdata_arm_patch = "${path.module}/templates/arm-runner-patch.tpl"
userdata_install_config_runner = "${path.module}/templates/install-config-runner.sh"

instance_types = var.instance_types == null ? [var.instance_type] : var.instance_types
instance_types = distinct(var.instance_types == null ? [var.instance_type] : var.instance_types)

kms_key_arn = var.kms_key_arn != null ? var.kms_key_arn : ""
}
Expand All @@ -38,9 +38,9 @@ data "aws_ami" "runner" {
}

resource "aws_launch_template" "runner" {
for_each = local.instance_types
count = length(local.instance_types)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change from loop to count?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You can't for_each on an array without converting it into a Set, which breaks the point of this PR ;).

I could have converted it into a map, but this seemed cleaner.

The given "for_each" argument value is unsuitable: the "for_each" argument must be a map, or set of strings, and you have provided a value of type tuple.

Copy link

Choose a reason for hiding this comment

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

Won't this introduce a different type of problem, where if an item is added or removed from the list, the indexes will change and terraform will destroy and recreate them all?

Copy link
Member

Choose a reason for hiding this comment

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

No the instance type list is only configuration to start instances, so it will never impact current running onces.

Copy link

Choose a reason for hiding this comment

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

That's not quite what I was trying to get at though, I don't think... This is for the aws_launch_template resource, not the launched instances... If the user modifies the list of input instances types for an already applied state, and runs the apply again, won't terraform destroy and recreate all the launch templates? Or no because the instance_type field can be modified without recreating the resource?

Copy link

@lorengordon lorengordon Sep 30, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Ahh, however this is mapped to the name field also, and that does require replacement, so yes, this will destroy and recreate the launch templates when the list changes.

Copy link

Choose a reason for hiding this comment

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

Changing from for_each to count is also a backwards-incompatible change for any existing state.


name = "${var.environment}-action-runner-${each.value}"
name = "${var.environment}-action-runner-${local.instance_types[count.index]}"

dynamic "block_device_mappings" {
for_each = [var.block_device_mappings]
Expand Down Expand Up @@ -68,7 +68,7 @@ resource "aws_launch_template" "runner" {
}

image_id = data.aws_ami.runner.id
instance_type = each.value
instance_type = local.instance_types[count.index]
key_name = var.key_name

vpc_security_group_ids = compact(concat(
Expand Down
2 changes: 1 addition & 1 deletion modules/runners/variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ variable "instance_type" {

variable "instance_types" {
description = "List of instance types for the action runner."
type = set(string)
type = list(string)
default = null
}

Expand Down
2 changes: 1 addition & 1 deletion variables.tf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -351,7 +351,7 @@ variable "volume_size" {

variable "instance_types" {
description = "List of instance types for the action runner."
type = set(string)
type = list(string)
default = null
}

Expand Down