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

AUT-4030: fix tfvars files #5819

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open

Conversation

whi-tw
Copy link
Contributor

@whi-tw whi-tw commented Jan 28, 2025

What

  • Tidy *.tfvars and variables.tf files.
  • Remove unneded /duplicated variables
  • build zips into terraform artifacts dirs, where they're located in CI.

How to review

  • Check no variables have been removed
  • check new defaults are sensible
  • check that deploying dev still works

Related PRs

Rebased onto #5823 and #5641

Copy link

github-actions bot commented Jan 28, 2025

Java Tests Not Skipped

Java files were previously skipped in this pull request. Subsequent changes have caused the tests to be run.

@whi-tw whi-tw force-pushed the whi-tw/AUT-4030/un-split-tfvars branch 2 times, most recently from b442a10 to 2de628f Compare January 29, 2025 15:06
Previously, when a P1 alarm was not needed, this would deploy on top of
module.account_interventions.aws_cloudwatch_metric_alarm.lambda_error_cloudwatch_alarm
so the configuration of the alarm would be modified each time. Now, we
only deploy the P1 alarm when required, and it will always be created
with a specific name.
This resource should only be set once per account, so it should:
- live in `shared`, so multiple components don't try to create it
- only be created by the 'primary' environment in the account, so that
  it's only created once

As the policy allows the api gateways to write any logs, it only needs
to be created here, not in each component.
Some modules have empty sections in their documentation. This change
hides those sections to make the documentation more concise.
As-was, this constantly attempted to change the policy resource to
'execute-api:/*', but AWS automatically replaced 'execute-api' with the
execution arn. This caused this resource to update on every terraform
run.
This replace function is, apparently, non-deterministic. This causes
some attributes to be marked as 'known after apply' when they shouldn't.

This change adds a new variable, `endpoint_name_sanitized`, which is
required if `endpoint_name` contains a period. This variable is then
used in place of `endpoint_name` in the resources that were using the
`replace` function.
Also, check all of the modules, not just two of them!
If terraform validate fails, the deploy in CI will fail. therefore it
makes sense to validate it here. As the validation is done at the same
time as the java build, it will not slow down this workflow.

The original terraform validate step in `pre-merge-checks-terraform.yml`
has been moved to a `workflow_call` workflow - allowing the same logic
to be run both in pre-merge-checks, and in the dev deploy workflow.
It's annoying to have to keep re-init'ing all the terraform modules
every time terraform is run. This change makes it so that the terraform
data dir is retained between runs of the same branch, so that the
`init` step takes less time on subsequent runs.

The data dirs can be removed by passing `-c` or `--clean` to the script.

These data dirs are stored in `${TMPDIR}/authentication-api-tf/${b64_branch_name}`,
where `${b64_branch_name}` is the base64 encoded branch name.

They will be removed on every reboot, so there's no need to worry about
regular cleanup.
@whi-tw whi-tw force-pushed the whi-tw/AUT-4030/un-split-tfvars branch 7 times, most recently from a60eec3 to 48a32ef Compare January 29, 2025 16:44
@whi-tw whi-tw force-pushed the whi-tw/AUT-4030/un-split-tfvars branch from 48a32ef to 0cff5a2 Compare January 29, 2025 16:48
@whi-tw whi-tw force-pushed the whi-tw/AUT-4030/un-split-tfvars branch from 0cff5a2 to a83dfbe Compare January 29, 2025 16:51
@whi-tw whi-tw force-pushed the whi-tw/AUT-4030/un-split-tfvars branch from a83dfbe to 989c75c Compare January 29, 2025 17:05
whi-tw added 18 commits January 29, 2025 17:10
Previously, the `buildZip` tasks would build the zip into gradle's
standard `build` directory. This is not the same location they are
located when running in CI.

This change updates the `buildZip` tasks to build the zip into the
`ci/terraform/<service>/artifacts` directory when the `devDeployBuild`
property is set. This property is set in the `deploy-dev.sh` script, and
should never need to be set manually.
- Remove orphaned variables
- Set defaults to the ones we want to use in dev environments
- Remove variables that set the variable to the default from
  variables.tf
Rather than having '*-overrides' and '*-sizing' files, instead just
create one file, with comments for headers.
Set defaults to the ones for dev envs, override them for non-dev
- Don't set values that are provided from defaults
- Don't explicitly set the value to the default from variables.tf
- Break up into sections
Merge sizing / overrides into main `.tfvars` files for each environment.

Also, remove vestigial variables.
- remove unused variables
- put variables into sections
- Set default lambda_min_concurrency to 0 (and set back to 1 where needed)
- Remove unnecessary definitions from tfvars (ie. re-setting defaults)
- Add tfvars into sections
@whi-tw whi-tw force-pushed the whi-tw/AUT-4030/un-split-tfvars branch from 989c75c to fe42dea Compare January 29, 2025 17:17
@whi-tw whi-tw marked this pull request as ready for review January 29, 2025 17:23
@whi-tw whi-tw requested review from a team as code owners January 29, 2025 17:23
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.

1 participant