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

Refactor content store #109

Merged
merged 5 commits into from
Jan 8, 2021
Merged

Refactor content store #109

merged 5 commits into from
Jan 8, 2021

Conversation

richardTowers
Copy link
Contributor

@richardTowers richardTowers commented Dec 18, 2020

This refactors the app task definition for content-store (and draft-content-store) from:

   deployments/apps/content-store
               |
             calls
               |
               V
modules/task-definitions/content-store
               |
             calls
               |
               V
    modules/task-definition
               |
            creates
               |
               V
     aws_ecs_task_definition

to:

----deployments/apps/content-store---------------------------------------------------->
               |                             |                             |
             calls                         calls                         creates
               |                             |                             |
               V                             V                             V
modules/app-task-definition        modules/envoy-settings       aws_ecs_task_definition

The configuration in main.tf is the same in both draft and live stacks. Mostly it loads stuff from remote state (from the govuk-test deployment, the mongo deployment from govuk-aws) and AWS data sources (for secrets manager).

draft.tf and live.tf call the app-task-definition and envoy-settings modules using the locals from main.tf, overriding them with anything that's different in this stack (e.g. by using merge() to override a couple of entries in the environment variables hash).

Chris and I discussed merging the app-task-definition and envoy-settings modules - we're on the fence about whether to do that. They're kind of separate, but in practice every govuk app is going to call both, so it might save some boilerplate if we merged them.

draft.tf and live.tf each create the aws_ecs_task_definition resource directly. In my view this is easier to follow than creating the resource inside a module (you can look at this deployment in isolation and know exactly what it's going to create, without having to open all of its modules too). It also means that modifying the task definition for an individual app is easier - things that are defined at the top level (like CPU) don't need to be exposed as variables in the module and passed down. That said, this is a bit of an unusual approach - we might decide that this is also too confusing.

The symlinked common.tf, outputs.tf and variables.tf files are not used in this deployment. I've also removed the hardcoded backend bucket for the test environment. That means that instead of:

terraform init
terraform apply -var-file=../../variables/test/app.tf -var-file=../variables/test/common.tf

The process for applying terraform is now:

terraform init -backend-config="bucket=govuk-terraform-test"
terraform apply -var="image_tag=deployed_to_production" -var="environment=test"

For now, this breaks the concourse pipeline for content-store and draft-content-store, as they've not been updated to use this module yet. I'm going to address that in a future PR, so I've paused those jobs in the pipeline.

I've already run terraform destroy on the old content-store and draft-content-store deployments, and I've run terraform apply for the new one.

https://trello.com/c/u36XaWOu/320-refactor-terraform-app-deployments

@richardTowers richardTowers force-pushed the refactor-content-store branch 3 times, most recently from ec9c152 to dba1648 Compare January 4, 2021 18:38
@richardTowers richardTowers marked this pull request as ready for review January 4, 2021 18:49
Copy link
Contributor

@sengi sengi left a comment

Choose a reason for hiding this comment

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

I think this is a massive improvement - go for it!

}

resource "aws_ecs_task_definition" "live" {
family = "content-store"
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this should be live-content-store, since we have the other family as draft-content-store and that name scheme used throughout.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would we then put the live- prefix on everything though? What about services which don't have a draft version? Would that make it clearer or more obscure for someone encountering this for the first time?

Copy link
Contributor

@sengi sengi Jan 5, 2021

Choose a reason for hiding this comment

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

I think this might be a case where the least surprising thing to do is to keep using the same names as before, e.g. plain content store is "content-store" and draft is "draft-content-store". What do you think? (We can always decide to rename things later if we think it really would be clearer, up until we go to production.)

Copy link
Contributor

@bilbof bilbof Jan 5, 2021

Choose a reason for hiding this comment

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

Yeah I agree - I think we should drop the "live-" prefix throughout though, to keep it consistent

Copy link
Contributor

@bilbof bilbof left a comment

Choose a reason for hiding this comment

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

Nice 👍

This will replace the old content-store and draft-content-store
deployments.

It uses the new side-effect-free app-container-definition and
envoy-configuration modules, instead of the
task-definitions/content-store and task-definition modules.

Once this has actually replaced the other deployments, we should delete
task-definitions/content-store (but not task-definition).
This will need to be passed in with:

```
terraform init -backend-config="bucket=govuk-terraform-test"
```

... which makes this configuration environment agnostic.
With the new-style app task definition deployment.

The content-store task-definitions module is no longer required.

Note: I've already run `terraform destroy` on the two removed
deployments.
@richardTowers richardTowers force-pushed the refactor-content-store branch from dba1648 to abf07d0 Compare January 8, 2021 13:59
@richardTowers richardTowers merged commit a7d7fdf into main Jan 8, 2021
@richardTowers richardTowers deleted the refactor-content-store branch January 8, 2021 14:38
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