flyte
flyte copied to clipboard
[flyte-core] Create separate grpc service for flyteadmin
Tracking issue
https://github.com/flyteorg/flyte/issues/4962
Why are the changes needed?
This allows setting annotations that are required for some ingress controllers for grpc communication only on parts that actually use grpc. Without this separation, either the console or the grpc endpoints did not work properly with some ingress controllers, e.g. traefik.
What changes were proposed in this pull request?
This PR splits the existing flyteadmin
service of the flyte-core
chart into two separate services (like in the flyte-binary
chart).
One service for grpc communication and another one for all other communication.
Accordingly, the annotations
field of the values.yaml
was extended to also use httpAnnotations
and grpcAnnotations
.
Technically, this is a breaking change for contour users, because the values.yaml
contained a default annotation
(that I think should not really be there anyway):
annotations:
projectcontour.io/upstream-protocol.h2c: grpc
I removed this default, but we could also keep this for backwards compatibility.
Docs link
I'm not sure how to regenerate the docs for this chart from the values.yaml
. Is there some command I can run?
Thank you for opening this pull request! 🙌
These tips will help get your PR across the finish line:
- Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
- Sign off your commits (Reference: DCO Guide).
I'm not sure how to regenerate the docs for this chart from the values.yaml. Is there some command I can run?
I think make helm
should do the trick
Codecov Report
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 59.95%. Comparing base (
6d58c73
) to head (44eaba8
). Report is 1 commits behind head on master.
:exclamation: Current head 44eaba8 differs from pull request most recent head 276679c. Consider uploading reports for the commit 276679c to get more accurate results
Additional details and impacted files
@@ Coverage Diff @@
## master #5168 +/- ##
===========================================
- Coverage 76.69% 59.95% -16.75%
===========================================
Files 18 463 +445
Lines 1515 37490 +35975
===========================================
+ Hits 1162 22477 +21315
- Misses 289 13329 +13040
- Partials 64 1684 +1620
Flag | Coverage Δ | |
---|---|---|
unittests | 59.95% <ø> (-16.75%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
I think we should make this optional (just as in flyte-binary) and probably leave the default behavior to be single service. wdyt?
Sounds good. I'll give it a shot.
I think we should make this optional (just as in flyte-binary) and probably leave the default behavior to be single service. wdyt?
This got quite big, because I tried to make it kinda similar to the flyte-binary
chart. wdyt?
@Jeinhaus any chance you can take a look at the comments? Thanks for putting so much effort into this!
This seems quite far and would be great to have! @Jeinhaus any chance you get to work on this still?