feat: Replace environment variable by prefix

[surminus]

We're looking at using this module to deploy multiple sets of runner types (x86_64 and arm architectures) but within the same conceptual "environment".

We use the "Environment" tag throughout our tooling, but the constraints of using the "environment" variable for resource naming mean that we need to essentially supply different environment names (eg "env-amd64" and "env-arm64"), even though they are not in different environments.

We also use the dot character (".") in our environment names, which isn't allowed in some resource names (eg SQS queue name).

This PR replaces the "environmet" variable by "prefix" to prefix resources crated by the module. The prefix is also used to set the tag: "ghr:environment" with the value of prefix for lambda's to orchestrate the instnaces.

You can still set the tag "environment" to all resources via the AWS provider.

[npalm]

@surminus can you push any tiny commit, the ci (actions) did not run. Most likely since github actions was down the last days twice.

[surminus]

Done, I amended the commit :)

[ScottGuymer]

We have been discussing removing the environment tag completely from the setup as it is too generic and causes too many clashes in users tagging schemas.

We have already moved the user-data scripting to use a namespaced tag. I think we should take this opportunity to remove the Environment tag completely from the module.

We can then just move to having a "prefix" variable that is used for the resource naming and the environment value will be specific and namespaced to the module itself.

Then users can use the providers tagging config to tag all resources with whichever tagging scheme they wish and we hopefully remove any clashes.

I think the Environment tag may be used in some of the lambdas and would need pointing to the ghr: namespaced environment tag.

[surminus]

OK, I think that makes sense... I'll take a blast at making these changes.

[npalm]

@surminus did you already had time to have a look?

[surminus]

Sorry, I started to look and then got a bit stuck in the weeds, since removing this tag entirely is a little more involved. I won't be able to look for at least a couple of weeks now, but I do intend to get back to this eventually!

[npalm]

No problem, thanks for the quick update!

[surminus]

@npalm I've pushed an update that completely removes the environment variable and replaces it where possible with var.prefix.

My main conundrum is how to pass in the ghr:environment tag to places that need it. At the moment I've added a lookup on the tags for this, but am aware that we're not enforcing it's presence, so may be prone to failure. Do we have any thoughts on the best way to pass this along between modules?

I've also added a default prefix as github-actions - let me know if you think this should be a required setting.

This update will also introduce breaking changes to users environments. They'll need to add an Environment tag to var.tags and specifically set the prefix to match their previous names.

[ScottGuymer]

You are right lookup(local.tags, "ghr:environment") definitely feels fragile.

The original implementation used the environment value (now prefix) as the value for the tag. I can see that this change decouples the prefix from the tag and uses a random value. Was there a reason to make this decoupling?

I think to keep the decoupling and not rely on the lookup would need new variables for the internal modules that use the tag value.

[surminus]

The original implementation used the environment value (now prefix) as the value for the tag. I can see that this change decouples the prefix from the tag and uses a random value. Was there a reason to make this decoupling?

Yes, I felt that it would be nicer to generate a completely new random value rather than depend on the prefix, which didn't feel like it's purpose. However, given that we'd have to create a new variable in all the downstream modules, I will just set it to var.prefix 👍

[surminus]

@npalm @ScottGuymer I've now updated this so that we're just using var.prefix everywhere.

[ScottGuymer]

Just one small thing to update

[surminus]

Thanks @ScottGuymer, I've updated this now. Let me know if it looks OK.

[ScottGuymer]

closes #1280

[npalm]

@surminus thanks for the work. Most of the changes looks good. The environment variable is used in the lamda's logic. So simpley removing will break some funciton of the module.

I also would prefer not to dropt direct the environment variable in the the variables.tf But see if we can provide a better migration part the consumers of the module. For example using a terraform validation rule for varialbes to indicate users how to migrate.

[surminus]

I also would prefer not to dropt direct the environment variable in the the variables.tf But see if we can provide a better migration part the consumers of the module. For example using a terraform validation rule for varialbes to indicate users how to migrate.

I originally wrote this using an optional prefix, but was then asked to remove the environment variable completely (unless I misunderstood).

Can I confirm, you would like to see the environment variable added back in, but with a validation rule that always evaluates to false, and explains how to migrate? I'll look at adding a commit that does this.

Can I also confirm about the Lambda ENVIRONMENT variable? This is still being passed a value.

[npalm]

I also would prefer not to dropt direct the environment variable in the the variables.tf But see if we can provide a better migration part the consumers of the module. For example using a terraform validation rule for varialbes to indicate users how to migrate.

I originally wrote this using an optional prefix, but was then asked to remove the environment variable completely (unless I misunderstood).

Can I confirm, you would like to see the environment variable added back in, but with a validation rule that always evaluates to false, and explains how to migrate? I'll look at adding a commit that does this.

Can I also confirm about the Lambda ENVIRONMENT variable? This is still being passed a value.

I merged the branch in a local branch up-to-date with development, for some reason something went wrong and I was in the impression the ENVIRONMENT variable was gone for the lambda's. Which isn't so. that comment can be ingored. Will resolve the comments.

Indeed, I would like to have the variable back for some time. So when uers are upgrading and have the environment set the got a clear messag how to migrate to the prefix. Or any other option that avoid users to get a hard time. I prefer to avoid a breaking release as well.

[surminus]

OK, will investigate the Lambdas and see how they use the Environment tag 👍

[npalm]

See: surminus#1

Tested: scaling up and down works, pool not checked.

[surminus]

Great, thank you! I've merged that in 🎉

[npalm]

Will run once more test today

[npalm]

LGTM!

Tested:

• scale down and up
• pool with ephemeral
• migration

[npalm]

@surminus updated the PR description and tittle

[surminus]

@npalm amazing thank you for spending the time to test it!

[ScottGuymer]

🚀 !

[sammort]

Thanks for the change @surminus my current Organization (with whom I think you are familiar 😉 ) will find this very useful.

[surminus]

Thanks for the change @surminus my current Organization (with whom I think you are familiar wink ) will find this very useful.

You're welcome @sammort, hope all is well over there!