STRRL / cloudflare-tunnel-ingress-controller

🚀 Expose the website directly into the internet! The Kuberntes Ingress Controller based on Cloudflare Tunnel.
MIT License
644 stars 37 forks source link

Update `Ingress.status.loadBalancer.ingress` when reconciling #69

Closed UnstoppableMango closed 5 months ago

UnstoppableMango commented 9 months ago

Should resolve #68

A note about the implementation, I wasn't a fan of mutating the Service inside of FromIngressToExposure but it was the most convenient place for it right now. I could move the mutation into the reconcile method directly, but then we would need a redundant client.Get() call. We could move it somewhere else as well, but that would probably require a bit of refactoring.

codecov-commenter commented 9 months ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (c685e37) 25.91% compared to head (c7ed1c5) 26.48%. Report is 1 commits behind head on master.

Files Patch % Lines
pkg/controller/transform.go 76.92% 2 Missing and 1 partial :warning:

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #69 +/- ## ========================================== + Coverage 25.91% 26.48% +0.57% ========================================== Files 9 9 Lines 629 638 +9 ========================================== + Hits 163 169 +6 - Misses 456 458 +2 - Partials 10 11 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

STRRL commented 8 months ago

PTAL @UnstoppableMango, I would very much appreciate it if you could address these changes. 🥰🥰

UnstoppableMango commented 8 months ago

You got it! That first one is a little embarrassing 😳 should be able to make the updates this afternoon!

UnstoppableMango commented 8 months ago

Ach I apoligize that first attempt was really half-baked!

I didn't see a great spot to move tests to, is there a place you think they should live?