-
Notifications
You must be signed in to change notification settings - Fork 652
Allow configuring additional security groups #392
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
Allow configuring additional security groups #392
Conversation
modules/runners/variables.tf
Outdated
@@ -278,3 +278,9 @@ variable "runner_log_files" { | |||
} | |||
] | |||
} | |||
|
|||
variable "additional_security_group_ids" { |
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.
Can you rename additional_security_group_ids
to runner_ additional_security_group_ids
whould be a bit more aligned with other variable names.
variables.tf
Outdated
@@ -296,3 +296,9 @@ variable "runner_log_files" { | |||
} | |||
] | |||
} | |||
|
|||
variable "additional_security_group_ids" { |
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.
Can you rename additional_security_group_ids
to runner_ additional_security_group_ids
whould be a bit more aligned with other variable names.
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!
For better readability, would it make sense to allow a user to pass in security group names, using this data source to then look up the IDs? |
This allows passing in additional security groups IDs to the launch template. We have a setup where we use a bastion host to connect to our instances, and would like to apply the same configuration to the runners.
I personally think using IDs is more idiomatic for Terraform resources, but I'm happy to make this change if the consensus is to use names instead. |
Just realised this got merged in a little while ago here. Thanks! 🎉 |
We've started recently looking to use this module and to fit it into our current infrastructure we'd like to be able to attach additional security groups to the runners. This will enable us to SSH to them which will help us debug some small customisations we're making in the user data.