[jellyfin] fix: jellyfin nodePort

onichandame commented 2 weeks ago

What this PR does / why we need it

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

fixes #

Special notes for your reviewer

Checklist

[x] Chart Version bumped
[x] Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

djjudas21 commented 2 weeks ago

Hey, thanks for your contribution. Please could you explain what this PR does / what bug it fixes?

I haven't used NodePort with Jellyfin before, but it looks like the line you're changing is inside a range block so maybe all the references to $.Values.service should be changed to $svc. I'll have another look at this tomorrow.

onichandame commented 2 weeks ago

Hey, thanks for your contribution. Please could you explain what this PR does / what bug it fixes?

I haven't used NodePort with Jellyfin before, but it looks like the line you're changing is inside a range block so maybe all the references to $.Values.service should be changed to $svc. I'll have another look at this tomorrow.

@djjudas21 Hi, the problem in this case is that the empty operator is applied to a should-not-exist field. imagine an example service config that comes from the default values:

service:
  type: NodePort
  web:
    port: 8096
    nodePort: 8096

In the original template (not (empty $.Values.service.nodePort)), it actually examines the nodePort field directly under service, which clearly does not exist. I'd imagine that the original intent was to examine whether service.web.nodePort exists instead.

The other field examining $.Values.service.type should be kept as is now, I think. By looking at the example config above, the type field should be directly under service(although I'm not sure whether it is a good design)

djjudas21 / charts