Skip to content
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

Closed
wants to merge 1 commit into from
Closed

Allow configuring additional security groups #392

wants to merge 1 commit into from

Conversation

surminus
Copy link
Contributor

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.

@npalm npalm self-requested a review December 14, 2020 09:01
@@ -278,3 +278,9 @@ variable "runner_log_files" {
}
]
}

variable "additional_security_group_ids" {
Copy link
Member

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" {
Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@jpalomaki
Copy link
Contributor

jpalomaki commented Dec 16, 2020

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.
@surminus
Copy link
Contributor Author

surminus commented Jan 4, 2021

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?

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.

@surminus
Copy link
Contributor Author

Just realised this got merged in a little while ago here. Thanks! 🎉

@surminus surminus closed this Jan 18, 2021
@sammort sammort deleted the lm-configurable-security-groups branch October 25, 2021 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants