Depends on D3692
Adding terraform configurations for the S3 and Dynamo DB.
Details
Details
- Reviewers
tomek • jim ashoat - Commits
- rCOMM474b3296954b: [services] Dev mode - Add terraform
cd services/terraform ./run.sh
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Wow, love how quickly you put this up, @karol-bisztyga!!
| services/terraform/dynamodb-test.tf | ||
|---|---|---|
| 66–79 | Why is this commented out? Let's avoid committing commented-out code | |
| services/terraform/.gitignore | ||
|---|---|---|
| 1–34 | Is this file copied from somewhere? | |
| services/terraform/dynamodb-test.tf | ||
| 66–79 | Agree. Is there a reason for including it? | |
| services/terraform/dynamodb.tf | ||
| 5–6 | Does that mean that only 10 clients can connect to the db at a time? Is our db configured in the same way currently? | |
| services/terraform/s3.tf | ||
| 8 | I'm not sure if we should include personal buckets here | |
| services/terraform/.gitignore | ||
|---|---|---|
| 1–34 | Yes, from here https://github.com/github/gitignore/blob/d0b80a469983a7beece8fa1f5c48a8242318b531/Terraform.gitignore Sorry I didn't post a source earlier. | |
| services/terraform/dynamodb-test.tf | ||
| 66–79 | We can remove them. I just thought we could have them so we later know the "full schema". The problem is, terraform doesn't want a full schema, it just wants the necessary minimum set of fields. So, I don't have a problem having them here but if you think they shouldn't be here, I can remove them. | |
| services/terraform/dynamodb.tf | ||
| 5–6 | From the docs:
Yes, this is copied from the settings on our cloud. At a first glance, it does not seem to be plenty. I think we can get back to this when doing performance tests. I created a task for this: https://linear.app/comm/issue/ENG-1054/think-about-readwrite-capacity-for-dynamodb-tables | |
| services/terraform/s3.tf | ||
| 8 | Hmm, right, I just copied what was on the cloud. Actually, I think we should only have commapp-blob here. | |
| services/terraform/s3.tf | ||
|---|---|---|
| 8 | Let's change this then? Back to your queue | |
Comment Actions
Accepting, but I don't have much context here – would be great if @jimpo could take a look
Comment Actions
@jimpo when can we expect a review of this? You're a blocking reviewer and the rest of this stack's accepted. Thanks.
Comment Actions
You should add a required_providers declaration somewhere like providers.tf. See https://registry.terraform.io/providers/hashicorp/aws/latest/docs.
Also add a package.json npm script to run terraform/run.sh, right?
| services/terraform/localstack.tf | ||
|---|---|---|
| 2 ↗ | (On Diff #12029) | This should have a default value as the port mapping is defined in docker-compose.yml |
| 5 ↗ | (On Diff #12029) | Please add comment with link to terraform documentation on localstack https://registry.terraform.io/providers/hashicorp/aws/latest/docs/guides/custom-service-endpoints#localstack |
| services/terraform/run.sh | ||
| 7 ↗ | (On Diff #12029) | Prefer -var "HOST=http://localhost:4566" to TF_VAR_HOST env var |
Comment Actions
You should add a required_providers declaration somewhere like providers.tf. See https://registry.terraform.io/providers/hashicorp/aws/latest/docs.
Sure, thanks!
Also add a package.json npm script to run terraform/run.sh, right?
It's added in D3694
| services/terraform/localstack.tf | ||
|---|---|---|
| 2 ↗ | (On Diff #12029) | I'm not sure why. We make sure to set this variable in the running bash script. I also do not understand what port mapping in docker-compose has to do with this. |
| 5 ↗ | (On Diff #12029) | Why should we do this? What is so unclear here? If something really is, the code reader can easily google the necessary information. We don't add comments to every piece of config/code that uses a certain technology (the same we could stuff up Dockerfiles or docker-compose.yml with links to docker docs). |
| services/terraform/run.sh | ||
| 7 ↗ | (On Diff #12029) | Ok, will change, personally, there is no difference for me (yeah, maybe -var is more readable) |
| services/terraform/localstack.tf | ||
|---|---|---|
| 2 ↗ | (On Diff #12029) | The default value should be http://localhost:4566. The port mapping is relevant because it sets the port to 4566 so we can safely make it the default here. Why? So that the configuration isn't split between terraform and a wrapper script. I'm really not sure why you like to depend so heavily on two line wrapper scripts because it limits flexibility. What if I want to do terraform plan? Then I need to look in the script and figure out what extra stuff it's doing like setting variables to a static string to get my command right. Why not also put the region in the wrapper script? Because there's no good reason to put more config there. |
| 5 ↗ | (On Diff #12029) | To help future readers of the code and reviewers. As a reviewer it's something that I needed to look up and I'm almost certain that you got the config options from that page. If it was one flag like skip_credentials_validation that the reader shouldn't be expected to know it would also be nice to leave a comment with the rationale but you could more reasonably expect them to look it up. This is a set of options coming from a very particular source and it would be helpful to link to that source. Another general rule of them for documentation is that if a reviewer finds something unclear enough it request clarification or a link to docs, I generally add it because comments are cheap. |
| services/terraform/run.sh | ||
| 7 ↗ | (On Diff #12029) | That is the recommended way of setting variables from the command line. Environment variables are a fallback. https://www.terraform.io/language/values/variables#variables-on-the-command-line |
| services/terraform/localstack.tf | ||
|---|---|---|
| 2 ↗ | (On Diff #12029) | I see but then what's the point of providing this value at the same time in the bash script? I don't see why we would have it in two places at the same time. I think we can delete this in the bash script if we have it here as a default because in the script we're not overriding to a different value. |
| 5 ↗ | (On Diff #12029) | Ok, I'm going to add the comment as I personally do not mind. Curious for @ashoat's thoughts. |
Comment Actions
Awesome, this looks great!
| services/terraform/localstack.tf | ||
|---|---|---|
| 5 ↗ | (On Diff #12029) | Agree with @jimpo's thoughts here – if it's confusing enough that a reviewer asks for clarification, it's probably worth a code comment. The context in which I usually oppose single-line comments is when they're making up for poorly readable code... I always prefer restructuring code / renaming variables to writing comments |
| services/terraform/s3.tf | ||
| 9–10 ↗ | (On Diff #12285) | Why are there so many spaces before the =? |