PrefectHQ / prefect-helm

Helm charts for deploying Prefect Services
Apache License 2.0
100 stars 60 forks source link

Allow to define SpreadTopologyContraints in values for server and worker #399

Closed GuilleAmutio closed 4 weeks ago

GuilleAmutio commented 1 month ago

Hi,

This PR allows user to define the SpreadTopologyConstraints for the charts of server and worker.

This feature was requested in https://github.com/PrefectHQ/prefect-helm/issues/360

GuilleAmutio commented 1 month ago

Just to add, worker lint is failing and dont know exactly why.

I removed what i did on the worker, and still failed the lint. Could someone recheck? Thanks

mitchnielsen commented 1 month ago

Current error:

==> Linting charts/prefect-worker
[INFO] Chart.yaml: icon is recommended
Error:  values.yaml: Expected: valid schema, given: Invalid JSON
Error:  templates/: values don't meet the specifications of the schema(s) in the following chart(s):
prefect-worker:
Expected: valid schema, given: Invalid JSON
mitchnielsen commented 1 month ago

@GuilleAmutio you'll need two fixes here:

First, the indentation of the labels needs to be corrected:

diff --git a/charts/prefect-worker/templates/deployment.yaml b/charts/prefect-worker/templates/deployment.yaml
index 3cf8a3f..d92cd49 100644
--- a/charts/prefect-worker/templates/deployment.yaml
+++ b/charts/prefect-worker/templates/deployment.yaml
@@ -229,7 +229,7 @@ spec:
               whenUnsatisfiable: {{ .whenUnsatisfiable }}
               labelSelector:
                 matchLabels:
-                  {{- toYaml .labelSelector.matchLabels | nindent 12 }}
+                  {{- toYaml .labelSelector.matchLabels | nindent 18 }}
             {{- end }}
           {{- end }}
           volumeMounts:

Second, you'll need to fix the schema:

diff --git a/charts/prefect-worker/values.schema.json b/charts/prefect-worker/values.schema.json
index e3a2a04..1149b2d 100644
--- a/charts/prefect-worker/values.schema.json
+++ b/charts/prefect-worker/values.schema.json
@@ -492,11 +492,11 @@
                       }
                     }
                   }
-                }
+                },
+                "required": ["maxSkew", "topologyKey", "whenUnsatisfiable", "labelSelector"]
               },
-              "required": ["maxSkew", "topologyKey", "whenUnsatisfiable", "labelSelector"]
-            },
-            "description": "Array of constraints for topology spread of the pods."
+              "description": "Array of constraints for topology spread of the pods."
+            }
           }
         },
         "livenessProbe": {
GuilleAmutio commented 4 weeks ago

Hi @mitchnielsen sorry for the delay, fixed, thank you very much for the comment before!

GuilleAmutio commented 4 weeks ago

Closing this MR due to conflicts with email used. Gonna reopen