Updating terraform and k8s files adding redis... #35
Conversation
![greptile-apps-staging[bot]](https://avatars.githubusercontent.com/u/140149887?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Hi! Looks like you've reached your API usage limit. You can increase it from your account settings page here: app.greptile.com/settings/billing/code-review
💡 (5/5) You can turn off certain types of comments like style here!
17 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
There was a problem hiding this comment.
PR Summary
This pull request introduces significant updates to the Kubernetes and Terraform configurations for the TwentyCRM application, focusing on Redis integration and improved secret management.
- Added Redis deployment and service configurations in both Kubernetes manifests and Terraform files
- Implemented new persistent volume and claim for Docker data storage
- Updated server and worker deployments to use Redis for caching and message queuing
- Improved secret management by generating random tokens using Terraform's random_bytes resource
- Modified environment variables and dependencies in server and worker deployments to accommodate Redis integration
17 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings | Greptile
| - name : "BACKEND_SERVER_URL" | ||
| value: var.twentycrm_app_hostname |
There was a problem hiding this comment.
logic: BACKEND_SERVER_URL uses Terraform variable syntax, which is incorrect in this YAML file
| - name : "REDIS_HOST" | ||
| value: "twentycrm-redis.twentycrm.svc.cluster.local" | ||
| - name: "REDIS_PORT" | ||
| value: 6379 |
There was a problem hiding this comment.
syntax: REDIS_PORT value should be a string
| value: 6379 | |
| value: "6379" |
| containers: | ||
| - env: | ||
| - name: PORT | ||
| value: 6379 |
There was a problem hiding this comment.
logic: The environment variable block is misplaced. It should be inside the container spec, not as a separate container.
| containers: | |
| - env: | |
| - name: PORT | |
| value: 6379 | |
| containers: | |
| - image: redis/redis-stack-server:latest | |
| env: | |
| - name: PORT | |
| value: "6379" |
| stdin: true | ||
| tty: true |
There was a problem hiding this comment.
style: stdin and tty are set to true, which is unusual for a Redis container. Consider removing these unless specifically required.
| | <a name="input_twentycrm_server_data_mount_path"></a> [twentycrm\_server\_data\_mount\_path](#input\_twentycrm\_server\_data\_mount\_path) | TwentyCRM mount path for servers application data. Defaults to '/app/docker-data'. | `string` | `"/app/docker-data"` | no | | ||
| | <a name="input_twentycrm_redis_image"></a> [twentycrm\_redis\_image](#input\_twentycrm\_redis\_image) | TwentyCRM image for Redis deployment. This defaults to latest. | `string` | `"redis/redis-stack-server:latest"` | no | | ||
| | <a name="input_twentycrm_redis_replicas"></a> [twentycrm\_redis\_replicas](#input\_twentycrm\_redis\_replicas) | Number of replicas for the TwentyCRM Redis deployment. This defaults to 1. | `string` | `"1"` | no | | ||
| | <a name="input_twentycrm_server_data_mount_path"></a> [twentycrm\_server\_data\_mount\_path](#input\_twentycrm\_server\_data\_mount\_path) | TwentyCRM mount path for servers application data. Defaults to '/app/packages/twenty-server/.local-storage. | `string` | `"/app/packages/twenty-server/.local-storage"` | no | |
There was a problem hiding this comment.
syntax: Missing period at the end of the description
| | <a name="input_twentycrm_server_data_mount_path"></a> [twentycrm\_server\_data\_mount\_path](#input\_twentycrm\_server\_data\_mount\_path) | TwentyCRM mount path for servers application data. Defaults to '/app/packages/twenty-server/.local-storage. | `string` | `"/app/packages/twenty-server/.local-storage"` | no | | |
| | <a name="input_twentycrm_server_data_mount_path"></a> [twentycrm\_server\_data\_mount\_path](#input\_twentycrm\_server\_data\_mount\_path) | TwentyCRM mount path for servers application data. Defaults to '/app/packages/twenty-server/.local-storage'. | `string` | `"/app/packages/twenty-server/.local-storage"` | no | |
| persistent_volume_source { | ||
| local { | ||
| path = var.twentycrm_docker_data_pv_path | ||
| } | ||
| } |
There was a problem hiding this comment.
style: using local storage may limit portability and scalability. consider using a more flexible storage option if possible
| } | ||
|
|
||
| resource "random_bytes" "this" { | ||
| for_each = toset(local.tokens) |
There was a problem hiding this comment.
style: Consider using a more descriptive name than 'this' for the random_bytes resource
| variable "twentycrm_redis_replicas" { | ||
| type = string | ||
| default = "1" | ||
| description = "Number of replicas for the TwentyCRM Redis deployment. This defaults to 1." | ||
| } |
There was a problem hiding this comment.
style: The type for 'twentycrm_redis_replicas' is set to string, but it represents a number. Consider changing the type to 'number' for consistency with other replica variables.
| variable "twentycrm_redis_replicas" { | |
| type = string | |
| default = "1" | |
| description = "Number of replicas for the TwentyCRM Redis deployment. This defaults to 1." | |
| } | |
| variable "twentycrm_redis_replicas" { | |
| type = number | |
| default = 1 | |
| description = "Number of replicas for the TwentyCRM Redis deployment. This defaults to 1." | |
| } |
| variable "twentycrm_docker_data_mount_path" { | ||
| type = string | ||
| default = "/app/docker-data" | ||
| description = "TwentyCRM mount path for servers application data. Defaults to '/app/docker-data'." | ||
| } |
There was a problem hiding this comment.
logic: This variable seems to duplicate the functionality of 'twentycrm_server_data_mount_path' (lines 54-58) with a different default value. Consider consolidating these variables or clarifying their distinct purposes.
Also updated the way secrets are generated with Terraform and some code cleanup