Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support changing paths on the ingress #52

Merged
merged 2 commits into from
Jan 14, 2024
Merged

Conversation

UnstoppableMango
Copy link
Contributor

@UnstoppableMango UnstoppableMango commented Jan 3, 2024

This introduces a breaking change, if that is not preferred I have another branch with the same change in a non-breaking fashion.

Sorry for the PR spam 😅 I was happy to find a maintained chart for this

@ressu
Copy link
Owner

ressu commented Jan 3, 2024

Whoops, I saw this after I had already merged the other fix. But I think I actually prefer this solution over the current option.

This introduces a breaking change, if that is not preferred I have another branch with the same change in a non-breaking fashion.
@UnstoppableMango
Copy link
Contributor Author

You're good! I hadn't actually opened a PR for the alternative, it was just a branch in my fork. I've rebased to fix the merge conflict in the README!

@ressu
Copy link
Owner

ressu commented Jan 7, 2024

I kind of want to merge this as is. But I keep thinking about backwards compatibility. A quick way to fix that would be to do a check in the template whether it's a string or array.

Honestly, I don't know if it's worth the effort though.

@UnstoppableMango
Copy link
Contributor Author

I learned about kindIs today, I've updated the impl to use that to check for a string host. How does that look?

You're right, it probably wasn't worth the effort but I think it turned out pretty ok 😄

@ressu ressu merged commit 799972a into ressu:main Jan 14, 2024
1 check passed
@ressu
Copy link
Owner

ressu commented Jan 14, 2024

Looks good. Thanks!

@UnstoppableMango UnstoppableMango deleted the paths branch January 14, 2024 15:52
@heliochronix
Copy link

I noticed this results in the deployment template setting ADVERTISE_IP to a map[] value rather than the actual FQDNs:

         - name: ADVERTISE_IP
           value: |
-            https://plex.domain.tld
-            ,https://plex.domain.tld:443
+            https://map[host:plex.domain.tld]
+            ,https://map[host:plex.domain.tld]:443

Is that okay or should the deployment.yaml template be fixed?

@UnstoppableMango
Copy link
Contributor Author

That's probably not right, but I'm not 100% sure. I'll take a look. Sorry if I introduced a bug!

@UnstoppableMango
Copy link
Contributor Author

Yep, I broke it. Working on a fix!

@ressu
Copy link
Owner

ressu commented Jan 17, 2024

Oh right, the advertise ip gets adjusted based on the ingress address too. To be fair, the whole block could be simplified quite a bit as it's doing the same thing in 2 blocks.

{{- if (or (eq .Values.service.type "ClusterIP") (empty .Values.service.type)) }}
- name: ADVERTISE_IP
value: |
{{ if .Values.ingress.hosts }}https://{{ join ",https://" .Values.ingress.hosts }}{{ end }}
{{ if .Values.ingress.hosts }},https://{{ join ",https://" .Values.ingress.hosts }}:443{{ end }}
{{- else if eq .Values.service.type "LoadBalancer" }}
{{- if (or .Values.service.loadBalancerIP .Values.ingress.hosts (index .Values.service.annotations "dns.pfsense.org/hostname")) }}
- name: ADVERTISE_IP
value: |
{{ if .Values.service.loadBalancerIP }}https://{{ .Values.service.loadBalancerIP }}:32400{{ end }}
{{ if index .Values.service.annotations "dns.pfsense.org/hostname" }},https://{{ join ",https://" (splitList "," (index .Values.service.annotations "dns.pfsense.org/hostname")) }}:32400{{ end }}
{{ if .Values.ingress.hosts }},https://{{ join ",https://" .Values.ingress.hosts }}{{ end }}
{{ if .Values.ingress.hosts }},https://{{ join ",https://" .Values.ingress.hosts }}:443{{ end }}
{{- end }}
{{- end }}

UnstoppableMango added a commit to UnstoppableMango/ressu-kube-plex that referenced this pull request Jan 18, 2024
Moves ADVERTISE_IP logic to a helper function
Fixes the regression from ressu#52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants