Lumosviridi v0.30.0 kubernetes updates #21
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
This PR introduces Redis integration and persistent storage configurations for the TwentyCRM Kubernetes deployment, replacing the previous PostgreSQL-based message queue with Redis.
- Critical issue in
/packages/twenty-docker/k8s/manifests/deployment-redis.yaml: Contains duplicate container definitions with conflicting PORT environment variables - Missing storage backend configuration in
/packages/twenty-docker/k8s/manifests/pv-docker-data.yamlwhich is required for actual storage allocation - Security concern: Removed security context settings in
deployment-server.tfthat could impact application permissions - Token management change in
secret.tfgenerates random tokens on each apply, requiring careful consideration for authentication across deployments - Redis resource limits in
deployment-redis.tfare significantly higher than other services (1024Mi/2048Mi memory vs 256Mi/1024Mi)
💡 (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!
17 file(s) reviewed, 30 comment(s)
Edit PR Review Bot Settings | Greptile
| sessionAffinity: ClientIP | ||
| sessionAffinityConfig: | ||
| clientIP: | ||
| timeoutSeconds: 10800 |
There was a problem hiding this comment.
style: 3-hour session affinity timeout (10800 seconds) may be excessive for Redis connections. Consider reducing to 1-2 hours to prevent stale connections.
| name: twentycrm-redis | ||
| namespace: twentycrm | ||
| spec: | ||
| internalTrafficPolicy: Cluster |
There was a problem hiding this comment.
style: internalTrafficPolicy: Cluster is redundant when type is ClusterIP - this is the default behavior
| namespace: twentycrm | ||
| spec: | ||
| storageClassName: default | ||
| volumeName: twentycrm-docker-data-pv |
There was a problem hiding this comment.
style: explicitly binding to PV reduces flexibility - consider removing volumeName for dynamic provisioning
| spec: | ||
| storageClassName: default | ||
| capacity: | ||
| storage: 10Gi | ||
| accessModes: | ||
| - ReadWriteOnce | ||
| persistentVolumeReclaimPolicy: Retain |
There was a problem hiding this comment.
logic: Missing required storage backend configuration (hostPath, nfs, etc). PV needs to specify where and how the actual storage is provisioned.
| containers: | ||
| - env: | ||
| - name: PORT | ||
| value: 6379 | ||
| - image: redis/redis-stack-server:latest |
There was a problem hiding this comment.
logic: The configuration defines two separate containers incorrectly. The env section is defined as a separate container from the Redis container. This will cause deployment issues.
| 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.
style: duplicate mount path purpose with twentycrm_server_data_mount_path - consider consolidating or clarifying the difference between these paths
| } | ||
|
|
||
| spec { | ||
| replicas = var.twentycrm_redis_replicas |
There was a problem hiding this comment.
logic: var.twentycrm_redis_replicas is used but not defined in the variables.tf file shown in the context
|
|
||
| spec { | ||
| container { | ||
| image = var.twentycrm_redis_image |
There was a problem hiding this comment.
logic: var.twentycrm_redis_image is used but not defined in the variables.tf file shown in the context
| stdin = true | ||
| tty = true |
There was a problem hiding this comment.
style: stdin and tty are set to true but may not be necessary for a Redis container as it typically runs as a daemon
| resources { | ||
| requests = { | ||
| cpu = "250m" | ||
| memory = "1024Mi" | ||
| } | ||
| limits = { | ||
| cpu = "500m" | ||
| memory = "2048Mi" | ||
| } | ||
| } |
There was a problem hiding this comment.
style: memory requests and limits are quite high for a basic Redis instance - consider reducing unless there's a specific requirement
No description provided.