ressu / kube-plex

Scalable Plex Media Server on Kubernetes -- dispatch transcode jobs as pods on your cluster!
Apache License 2.0
104 stars 23 forks source link

Fix ADVERTISE_IP to work with structured ingress paths #53

Closed UnstoppableMango closed 8 months ago

UnstoppableMango commented 8 months ago

Moves ADVERTISE_IP logic to a helper function Fixes the regression from #52

I believe this should have roughly equivalent output to the original.

Here are a few of the commands I tested with helm template . --set 'ingress.enabled=true' --set 'ingress.hosts[0]=test.example.com' --set 'ingress.hosts[1]=test2.example.com' helm template . --set 'ingress.enabled=true' --set 'ingress.hosts[0].host=test.example.com' --set 'ingress.hosts[1].host=test2.example.com'

I noticed that on main the first url doesn't seem to get a :443 variant and is duplicated instead. Was this intentional?

Main renders:

- name: ADVERTISE_IP
  value: |
    https://test.example.com,https://test2.example.com
    ,https://test.example.com,https://test2.example.com:443

This branch renders (whitespace added for readability):

- name: ADVERTISE_IP
  value: |
    https://test.example.com,https://test.example.com:443
    ,https://test2.example.com,https://test2.example.com:443
ressu commented 8 months ago

I did a bit of a rework of all of the advertiseIp handling. Does it still work as intended for you? I tried to break it in all the ways I could think of and always ended up getting reasonable results.

UnstoppableMango commented 8 months ago

LGTM! @heliochronix is this good for you as well?

Sorry again for breaking it!

ressu commented 8 months ago

Heh, don't worry about breaking things. That's just part of the normal process.

heliochronix commented 8 months ago

From what I see, looks good. I'm no expert in helm templates though haha.