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

feat(chart): add service.yaml #253

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sanadhis
Copy link

What problem:
I need to install caddy to K8s, but I don't need external load balancer.

Why this is a solution:
By adding this loadBalancer.enabled value, which default to true, we can make the load balancer optional. This is also to ensure smart transition to service.yaml for already-existing user of this helm chart.

@mavimo
Copy link
Member

mavimo commented Sep 3, 2024

@sanadhis thanks for your contribution!

May I ask to include the new keys in the values.schema.json file?

Copy link

codecov bot commented Sep 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 21.89%. Comparing base (48ef36b) to head (25eb679).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #253      +/-   ##
==========================================
+ Coverage   18.34%   21.89%   +3.55%     
==========================================
  Files          30       30              
  Lines        1063     1087      +24     
==========================================
+ Hits          195      238      +43     
+ Misses        867      847      -20     
- Partials        1        2       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sanadhis
Copy link
Author

sanadhis commented Sep 3, 2024

@sanadhis thanks for your contribution!

May I ask to include the new keys in the values.schema.json file?

Thanks for your feedback!
Yes, updated

@mavimo
Copy link
Member

mavimo commented Sep 6, 2024

@sanadhis internalTrafficPolicy and externalTrafficPolicy have different behaviour (internal for traffic routing exteral for preserving client IP) I'm not sure about the last changes you included, can you give us some context? 🤔

Thanks!

@sanadhis
Copy link
Author

sanadhis commented Sep 6, 2024

@sanadhis internalTrafficPolicy and externalTrafficPolicy have different behaviour (internal for traffic routing exteral for preserving client IP) I'm not sure about the last changes you included, can you give us some context? 🤔

Thanks!

Hey, yes I am well aware, I was thinking to keep things simple. Sure I can add it back. I'll do some refactor also since we provide default value anyway. I am also squashing my commits

@sanadhis
Copy link
Author

@mavimo can we merge this one please? Thanks!

@mavimo
Copy link
Member

mavimo commented Sep 16, 2024

@sanadhis I'll try to run the validation and merge ASAP

I'm planning to release a new version as soon as this is merged.

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.

2 participants