Skip to content

VED-1040: Terraform tidy #1172

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

Jump to bottom
Open
edhall-nhs wants to merge 7 commits into
base: master
Choose a base branch
from
Open

VED-1040: Terraform tidy #1172

edhall-nhs wants to merge 7 commits into from
+128 −150

Conversation

@edhall-nhs
Copy link
Contributor

@edhall-nhs edhall-nhs commented Jan 29, 2026

Summary

  • Routine Change
  • ⚠️ Potential issues that might be caused by this change

This PR:

  • Brings all lambda names into alignment by using hyphens instead of underscores where there was a mix previously.
  • Reuses the lambda name local variable where possible.
  • Replaces hard coded AWS region with the global variable.

Since the names of some lambdas have changed this could have unwanted effects. They should be mapped accordingly but we should be careful.

Reviews Required

  • Dev
  • Test
  • Tech Author
  • Product Owner

Review Checklist

ℹ️ This section is to be filled in by the reviewer.

  • I have reviewed the changes in this PR and they fill all of the acceptance criteria of the ticket.
  • If there were infrastructure, operational, or build changes, I have made sure there is sufficient evidence that the changes will work.
  • If there were changes that are outside of the regular release processes e.g. account infrastructure to setup, manual setup for external API integrations, secrets to set, then I have checked that the developer has flagged this to the Tech Lead as release steps.
  • I have checked that no Personal Identifiable Data (PID) is logged as part of the changes.

@edhall-nhs
Consolidate lambda names to use hyphens
2fbfd6e 
Also tidy us hard coded aws regions and use vars instead
@github-actions
Copy link
Contributor

github-actions bot commented Jan 29, 2026

This branch is working on a ticket in the NHS England VED JIRA Project. Here's a handy link to the ticket:

VED-1040

@edhall-nhs edhall-nhs temporarily deployed to internal-dev-sandbox January 29, 2026 16:40 — with GitHub Actions Inactive
@edhall-nhs edhall-nhs temporarily deployed to internal-dev-sandbox January 29, 2026 16:41 — with GitHub Actions Inactive
@edhall-nhs edhall-nhs mentioned this pull request Jan 29, 2026
Closed
8 tasks
@edhall-nhs edhall-nhs marked this pull request as ready for review January 30, 2026 09:19
@edhall-nhs edhall-nhs temporarily deployed to internal-dev-sandbox January 30, 2026 09:22 — with GitHub Actions Inactive
@edhall-nhs edhall-nhs temporarily deployed to internal-dev-sandbox January 30, 2026 09:27 — with GitHub Actions Inactive
JamesW1-NHS
JamesW1-NHS requested changes Jan 30, 2026
View reviewed changes
@dlzhry2nhs dlzhry2nhs self-requested a review January 30, 2026 11:15
@edhall-nhs edhall-nhs temporarily deployed to internal-dev-sandbox January 30, 2026 11:16 — with GitHub Actions Inactive
@edhall-nhs edhall-nhs temporarily deployed to internal-dev-sandbox January 30, 2026 11:20 — with GitHub Actions Inactive
dlzhry2nhs
dlzhry2nhs reviewed Jan 30, 2026
View reviewed changes
dlzhry2nhs
dlzhry2nhs reviewed Jan 30, 2026
View reviewed changes
dlzhry2nhs
dlzhry2nhs reviewed Jan 30, 2026
View reviewed changes
infrastructure/instance/redis_sync_lambda.tf Outdated
REDIS_HOST = data.aws_elasticache_cluster.existing_redis.cache_nodes[0].address
REDIS_PORT = data.aws_elasticache_cluster.existing_redis.cache_nodes[0].port
REDIS_SYNC_PROC_LAMBDA_NAME = "imms-${var.sub_environment}-redis_sync_lambda"
REDIS_SYNC_PROC_LAMBDA_NAME = local.redis_sync_lambda_name
Copy link
Collaborator

@dlzhry2nhs dlzhry2nhs Jan 30, 2026

Choose a reason for hiding this comment

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

Do we use this environment variable anywhere? Same probably goes for the SPLUNK_FIREHOSE_NAME.

Copy link
Contributor Author

@edhall-nhs edhall-nhs Jan 30, 2026

Choose a reason for hiding this comment

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

REDIS_SYNC_PROC_LAMBDA_NAME doesn't appear to be used anywhere in the repo so will delete
I think SPLUNK_FIREHOSE_NAME will be needed as the handler is decorated with the logging decorator

dlzhry2nhs
dlzhry2nhs reviewed Jan 30, 2026
View reviewed changes
infrastructure/mesh/variables.tf
variable "mesh_mailbox_ids" {}
variable "mesh_dlq_mailbox_id" {}
variable "aws_region" {
default = "eu-west-2"
Copy link
Collaborator

@dlzhry2nhs dlzhry2nhs Jan 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Optional, probably not needed as is inconsistent with the other directories, unless we wanna apply elsewhere.

But terraform offers variable validation. We are only really ever meant to deploy NHS services into eu-west-2. Any developer could override ultimately (so there is limited value), so it relies on higher-level controls etc. But a simple validation could help to express intent.

https://developer.hashicorp.com/terraform/language/block/variable#validation

Copy link
Contributor Author

@edhall-nhs edhall-nhs Jan 30, 2026

Choose a reason for hiding this comment

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

I'll look into this. Sounds like it's worth adding since we're having a tidy 👍

Copy link
Contributor Author

@edhall-nhs edhall-nhs Jan 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

A note on this, I noticed we have some AWS shield related stuff set to us-east-1. I'm not too clued up on it so is this an exception and it needs to be in us-east-1?

Copy link
Collaborator

@dlzhry2nhs dlzhry2nhs Jan 30, 2026

Choose a reason for hiding this comment

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

Ah my bad. Forgot about those, yes some of those would be global services for the Cyber Security monitoring which are only available there. I think all the resources we deploy that use the variable are eu-west-2. But if it causes issues just dismiss and merge.

dlzhry2nhs
dlzhry2nhs previously approved these changes Jan 30, 2026
View reviewed changes
@JamesW1-NHS JamesW1-NHS self-requested a review January 30, 2026 11:44
JamesW1-NHS
JamesW1-NHS previously approved these changes Jan 30, 2026
View reviewed changes
@edhall-nhs edhall-nhs dismissed stale reviews from JamesW1-NHS and dlzhry2nhs via 8e63835 January 30, 2026 14:52
@edhall-nhs edhall-nhs had a problem deploying to internal-dev-sandbox January 30, 2026 14:55 — with GitHub Actions Failure
@edhall-nhs edhall-nhs temporarily deployed to internal-dev-sandbox January 30, 2026 15:13 — with GitHub Actions Inactive
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jan 30, 2026

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

@edhall-nhs edhall-nhs temporarily deployed to internal-dev-sandbox January 30, 2026 15:18 — with GitHub Actions Inactive
@edhall-nhs edhall-nhs deployed to internal-dev-sandbox January 30, 2026 15:18 — with GitHub Actions Active
@edhall-nhs edhall-nhs deployed to internal-dev January 30, 2026 15:21 — with GitHub Actions Active
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dlzhry2nhs dlzhry2nhs dlzhry2nhs left review comments

@JamesW1-NHS JamesW1-NHS JamesW1-NHS left review comments

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@edhall-nhs @dlzhry2nhs @JamesW1-NHS