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

Fix ADVERTISE_IP to work with structured ingress paths #53

Merged
merged 2 commits into from
Jan 19, 2024

Conversation

UnstoppableMango
Copy link
Contributor

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

UnstoppableMango and others added 2 commits January 17, 2024 21:07
Moves ADVERTISE_IP logic to a helper function
Fixes the regression from ressu#52
While this might look more complex, it breaks down to a set of providers for hostnames which then combine into a comma separated list. So it should be much easier to extend in future.
@ressu
Copy link
Owner

ressu commented Jan 18, 2024

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
Copy link
Contributor Author

UnstoppableMango commented Jan 18, 2024

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

Sorry again for breaking it!

@ressu
Copy link
Owner

ressu commented Jan 18, 2024

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

@heliochronix
Copy link

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

@ressu ressu merged commit f6d611c into ressu:main Jan 19, 2024
1 check passed
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