memgraph / helm-charts

Helm charts for deploying Memgraph, an open-source in-memory graph database.
https://memgraph.github.io/helm-charts/
Apache License 2.0
13 stars 9 forks source link

Resources, Service Accounts, labels and App Version #7

Closed jseiser closed 1 year ago

jseiser commented 1 year ago

fix(resources): Allow Setting Pod resources fix(service_account): Allow Creating Custom Service Account fix(labels): Use Label templates fix(app_version): Use and default to app version instead of latest

Closes: #6 Closes #4 Closes #3 Closes #2

jseiser commented 1 year ago
❯ helm template charts/memgraph | kubeconform --summary 
Summary: 6 resources found parsing stdin - Valid: 6, Invalid: 0, Errors: 0, Skipped: 0
alex-linx commented 1 year ago

@jseiser Awesome work, thanks! Just a small thing in #6 I also asked to have the option for annotations on the statefulset / service. If you could add it that would be amazing, if not I'll just open another issue/PR

jseiser commented 1 year ago

@jseiser Awesome work, thanks! Just a small thing in #6 I also asked to have the option for annotations on the statefulset / service. If you could add it that would be amazing, if not I'll just open another issue/PR

podAnnotations
serviceAccount.annotations
service.annotations
jseiser commented 1 year ago

Sorry, moving too fast. Ill get this fixed in the morning

On Tue, Aug 15, 2023, 4:16 PM alex-linx @.***> wrote:

@.**** commented on this pull request.

In charts/memgraph/templates/statefulset.yaml https://github.com/memgraph/helm-charts/pull/7#discussion_r1295067510:

@@ -4,53 +4,60 @@ kind: StatefulSet metadata: name: {{ include "memgraph.fullname" . }} labels:

  • app.kubernetes.io/name: memgraph
  • app.kubernetes.io/managed-by: Helm
  • {{- include "memgraph.labels" . | nindent 4 }}
  • {{- with .Values.podAnnotations }}

This should me moved under spec.template.metadata.annotations, and then have a separate statefulSet.annotations values

— Reply to this email directly, view it on GitHub https://github.com/memgraph/helm-charts/pull/7#pullrequestreview-1579350576, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABFBNZ7OIVUUGBTTRETVY23XVPKK5ANCNFSM6AAAAAA3REY34A . You are receiving this because you were mentioned.Message ID: @.***>

jseiser commented 1 year ago

@antejavor

Anything else you need me to do on this one? I know we are sitting in different timezones.

antejavor commented 1 year ago

@jseiser No at the moment, the changes look nice and are highly appreciated. Test charts failed for some reason, will look into it and we will probably merge this next week, and make a new release.