-
-
Notifications
You must be signed in to change notification settings - Fork 168
feat(#677): Helm chart to deploy HelixDB on Kubernetes #693
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
base: dev
Are you sure you want to change the base?
Conversation
|
Hi, I’ve just created the pull request. Please don’t hesitate to let me know if you notice anything that doesn’t look right so I can fix it. Regards. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7 files reviewed, 6 comments
k8s/helm/values.yaml
Outdated
| repository: "" | ||
| tag: "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: required fields repository and tag are empty - deployment will fail without these values
| repository: "" | |
| tag: "" | |
| repository: "your-registry/helixdb" | |
| tag: "2.1.2" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: k8s/helm/values.yaml
Line: 6:7
Comment:
**syntax:** required fields `repository` and `tag` are empty - deployment will fail without these values
```suggestion
repository: "your-registry/helixdb"
tag: "2.1.2"
```
How can I resolve this? If you propose a fix, please make it concise.
k8s/helm/values.yaml
Outdated
| # Optional storage class (default = cluster default) | ||
| storageClassName: "" | ||
| # Requested storage size | ||
| size: "" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: required field size is empty - PVC creation will fail without a storage size
| size: "" | |
| size: "10Gi" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: k8s/helm/values.yaml
Line: 20:20
Comment:
**syntax:** required field `size` is empty - PVC creation will fail without a storage size
```suggestion
size: "10Gi"
```
How can I resolve this? If you propose a fix, please make it concise.| securityContext: {} | ||
| containers: | ||
| - name: helixdb | ||
| image: "{{ .Values.image.repository }}:{{ .Values.image.tag }}" | ||
| imagePullPolicy: {{ .Values.image.pullPolicy }} | ||
| command: ["/usr/local/bin/helix-container"] | ||
| securityContext: {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: no health checks configured - Kubernetes won't know if the container is ready to accept traffic or needs to be restarted
Prompt To Fix With AI
This is a comment left during a code review.
Path: k8s/helm/templates/deployment.yaml
Line: 18:24
Comment:
**style:** no health checks configured - Kubernetes won't know if the container is ready to accept traffic or needs to be restarted
How can I resolve this? If you propose a fix, please make it concise.
k8s/README.md
Outdated
|
|
||
| ## Docker Image | ||
|
|
||
| The container image runs the the `helix-container` binary which was compiled with the helix push tool previously. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: typo: repeated "the"
| The container image runs the the `helix-container` binary which was compiled with the helix push tool previously. | |
| The container image runs the `helix-container` binary which was compiled with the helix push tool previously. |
Prompt To Fix With AI
This is a comment left during a code review.
Path: k8s/README.md
Line: 11:11
Comment:
**syntax:** typo: repeated "the"
```suggestion
The container image runs the `helix-container` binary which was compiled with the helix push tool previously.
```
How can I resolve this? If you propose a fix, please make it concise.
k8s/README.md
Outdated
| ## Build and Push | ||
|
|
||
| The Helm chart is located in the helm/ directory and provides a configurable Kubernetes deployment, service, and persistent volume claim. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: "Build and Push" section header has no content - either add content or remove the header
Prompt To Fix With AI
This is a comment left during a code review.
Path: k8s/README.md
Line: 13:15
Comment:
**style:** "Build and Push" section header has no content - either add content or remove the header
How can I resolve this? If you propose a fix, please make it concise.| {{- with .Values.resources }} | ||
| resources: | ||
| {{ toYaml . | indent 12 }} | ||
| {{- end }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
syntax: incorrect indentation for resources block - should be 12 spaces but toYaml outputs at wrong level
| {{- with .Values.resources }} | |
| resources: | |
| {{ toYaml . | indent 12 }} | |
| {{- end }} | |
| {{- with .Values.resources }} | |
| resources: | |
| {{ toYaml . | indent 12 }} | |
| {{- end }} |
Prompt To Fix With AI
This is a comment left during a code review.
Path: k8s/helm/templates/deployment.yaml
Line: 30:33
Comment:
**syntax:** incorrect indentation for resources block - should be 12 spaces but `toYaml` outputs at wrong level
```suggestion
{{- with .Values.resources }}
resources:
{{ toYaml . | indent 12 }}
{{- end }}
```
How can I resolve this? If you propose a fix, please make it concise.|
@amartin0 thoughts on greptile recommendations? |
|
Hi @xav-db , I’ve turned the PR into a draft for now. I’ll work on the greptile-apps comments over the weekend and re-activate it once it’s ready for your review. Thanks!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7 files reviewed, 4 comments
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
| metadata: | ||
| name: helixdb-service | ||
| namespace: {{ .Values.namespace }} | ||
| spec: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: missing explicit type: ClusterIP - while ClusterIP is the default, explicitly specifying it improves clarity and prevents confusion
Prompt To Fix With AI
This is a comment left during a code review.
Path: k8s/helm/templates/service.yaml
Line: 6:6
Comment:
**style:** missing explicit `type: ClusterIP` - while ClusterIP is the default, explicitly specifying it improves clarity and prevents confusion
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| ## Deploying to Kubernetes | ||
|
|
||
| helm install helixdb ./helm -n your-namespace --create-namespace |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: missing code formatting for command - should be wrapped in code block for better readability
Prompt To Fix With AI
This is a comment left during a code review.
Path: k8s/README.md
Line: 30:30
Comment:
**style:** missing code formatting for command - should be wrapped in code block for better readability
How can I resolve this? If you propose a fix, please make it concise.
k8s/helm/values.yaml
Outdated
|
|
||
| image: | ||
| # Image repository and tag | ||
| repository: "your-registry/helixdb" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: placeholder value your-registry/helixdb should be replaced with actual registry path before deployment
Prompt To Fix With AI
This is a comment left during a code review.
Path: k8s/helm/values.yaml
Line: 6:6
Comment:
**style:** placeholder value `your-registry/helixdb` should be replaced with actual registry path before deployment
How can I resolve this? If you propose a fix, please make it concise.| spec: | ||
| # Force ReadWriteOncePod mode | ||
| accessModes: | ||
| - ReadWriteOncePod |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
style: ReadWriteOncePod access mode requires Kubernetes 1.22+ - verify cluster version supports this feature
Prompt To Fix With AI
This is a comment left during a code review.
Path: k8s/helm/templates/pvc.yaml
Line: 9:9
Comment:
**style:** `ReadWriteOncePod` access mode requires Kubernetes 1.22+ - verify cluster version supports this feature
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7 files reviewed, no comments
Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format
|
Hi @xav-db The code has now passed the greptile-apps tests. Please note that I consider the data can be written by a single process, which is why I am enforcing the ReadWriteOncePod access mode on the PVC. Anything you consider needs to be modified, improved, or documented, please don’t hesitate to ask me. Thanks. |
|
this looks good to me. I will give it a proper review in the next day or so. @matthewsanetra probably a good idea to review this too! |
|
im going to make this a draft as im unsure about having k8s deployments in main binary repo |
Description
Add helm to deploy helixdb on kubernetes
Related Issues
677
Closes #
Checklist when merging to main
rustfmthelix-cli/Cargo.tomlandhelixdb/Cargo.tomlAdditional Notes
Greptile Summary
/introspectendpoint and configurable resource limitsImportant Files Changed
Sequence Diagram
sequenceDiagram participant User as "User/DevOps" participant Helm as "Helm CLI" participant K8s as "Kubernetes API" participant PVC as "PersistentVolumeClaim" participant Deploy as "Deployment" participant Pod as "HelixDB Pod" participant Service as "ClusterIP Service" User->>Helm: "helm install helixdb ./helm" Helm->>K8s: "Apply Chart.yaml metadata" Helm->>K8s: "Create namespace (if needed)" Helm->>K8s: "Create PVC (helixdb-data)" K8s->>PVC: "Provision storage with ReadWriteOncePod" PVC-->>K8s: "Storage ready" Helm->>K8s: "Create Service (helixdb-service)" K8s->>Service: "Expose port 6969 (ClusterIP)" Helm->>K8s: "Create Deployment (1 replica)" K8s->>Deploy: "Start deployment" Deploy->>Pod: "Create pod with helix-container image" Pod->>PVC: "Mount /data volume" Pod->>Pod: "Run helix-container as UID 1001" Pod->>Pod: "Initialize HELIX_DATA_DIR=/data" Pod-->>Deploy: "Pod running" Deploy-->>Service: "Register pod endpoint" Service-->>User: "helixdb-service:6969 ready"