PrefectHQ / prefect-helm

Helm charts for deploying Prefect Services
Apache License 2.0
83 stars 54 forks source link

prefect-server helm chart enhancement to support multi-port service #318

Closed baisystems closed 1 month ago

baisystems commented 2 months ago

prefect-server helm chart enhancement to support multi-port service.

closes https://github.com/PrefectHQ/prefect-helm/issues/314

jamiezieziula commented 2 months ago

Hi @baisystems, thanks for the PR! Is there a reason we can't accomplish this functionality by allowing for additionalPorts to be passed as an optional array[dictionary] and leaving the current handling of the required service ports as is?

baisystems commented 2 months ago

Hi @jamiezieziula, there is no additionalPorts currently. If I understand you correctly, you are proposing to add a new extraPorts field under service in values.yaml file and use it like this in service template:

ports:
    - name: server-svc-port
      port: {{ .Values.service.port }}
      ...
    {{- range .Values.service.extraPorts }}
    - name: {{ .name }}
      port: {{ .port }}
      ...
    {{- end }}

If this is the case, my answer is Yes, we can accomplish this functionality with extraPorts. I do see the merits of your proposal: minimal changes to current code base as I don't need to change deployment.yaml and I don't need to add a new function tartget-port.yaml.

Your proposal also agree well with your current helm chart pattern extra ..., for example, extraContainers for server pod and extraPaths for server ingress.

I actually like your idea and can tailor my pr to implement the extraPorts field as I illustrated above.

The second feature of this PR is its introduction of a new field servicePort to ingress. This will make the ingress service port configurable to accommodate the case where the ingress is serving a port other than server-svc-port. What do you think about this change?

jamiezieziula commented 2 months ago

@baisystems thanks for the feedback! I look forward to reviewing your changes :)

I like the idea of the customizability of the ingress service port!

baisystems commented 2 months ago

@jamiezieziula I pushed a new commit which implemented the extraPorts field as discussed. Please review.

baisystems commented 1 month ago

@jamiezieziula I pushed a new commit which default the extraPorts value to []. Any other issue for this pr?

jamiezieziula commented 1 month ago

@baisystems - can you install precommit and run it? linters are failing.

baisystems commented 1 month ago

@jamiezieziula I pushed a new commit. All the lint errors are fixed and the build is now passed. Could you please take another look?

jamiezieziula commented 1 month ago

Hi @baisystems - thanks! All linting looks healthy now. Sorry to do this, but one last thing - see @mitchnielsen's comment above.

baisystems commented 1 month ago

@jamiezieziula I tested @mitchnielsen's suggestion, helm install succeed with it. I will push an update commit soon.

baisystems commented 1 month ago

@jamiezieziula I pushed a new commit, the 5th. Please review again.