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

Reduce the length of the EC2 runner role and instance profile names #1340

Closed
toast-gear opened this issue Oct 26, 2021 · 1 comment · Fixed by #1358
Closed

Reduce the length of the EC2 runner role and instance profile names #1340

toast-gear opened this issue Oct 26, 2021 · 1 comment · Fixed by #1358
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@toast-gear
Copy link
Contributor

toast-gear commented Oct 26, 2021

Horse may have bolted on this one as it would be a breaking change however I think it's worthwhile doing.

https://github.com/philips-labs/terraform-aws-github-runner/blob/develop/modules/runners/policies-runner.tf#L4
https://github.com/philips-labs/terraform-aws-github-runner/blob/develop/modules/runners/policies-runner.tf#L11

The current role and instance profile name is quite verbose. This doesn't cause a technical issue however it isn't a great end user experience. We prefix the role name with the environment which already has to be unique and so having such a long name is unnecessary and makes self-service a bit of a faff.

Perhaps something less verbose like "${var.environment}-runner-role | "${var.environment}-runner-profile" (my preferred) or "${var.environment}-actions-runner-role | "${var.environment}-actions-runner-profile". Tags are the preferred way of categoring and filtering resources and so I don't think we need to go too overboard on the name, we just need to ensure it will be unique for multiple deployments of this stack and it relates to the GitHub Actions platform.

Further more, paths are already supported and so it seems unnecessary having such a long name. I know changing this is a breaking change which will have implications on people's IAM trust relationships however this is easily enough to fix in the deployment role by just adding a second trust relationship in preperation to upgrading their deployments of this module and so I think it's worth simplifying for the sake of the long term health of the project.

If you don't want to release a breaking change (I think sometimes this is actually the better long term option and something to consider) then a set of optional variables to define the role name and instance profile name will do.

@toast-gear toast-gear changed the title Reduce the length of of the EC2 runner role name Reduce the length of of the EC2 runner role and instance profile name Oct 26, 2021
@toast-gear toast-gear changed the title Reduce the length of of the EC2 runner role and instance profile name Reduce the length of the EC2 runner role and instance profile names Oct 26, 2021
@npalm npalm added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Oct 26, 2021
@toast-gear
Copy link
Contributor Author

toast-gear commented Oct 26, 2021

As this is an enhancement are you open to reducing the name length statically creating a breaking change or do you want it to be a backwards compatible change by introducing a variable? The latter would work but it feels very awkward as this isn't something you would wrap in a variable if the project wasn't already used.

EDIT @npalm details on how you want the enhacnement done would be great to hear

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants