-
Notifications
You must be signed in to change notification settings - Fork 665
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 separateGrpcIngress flag not working in flyte-binary helm chart #4946
Conversation
Signed-off-by: Ryan Lo <[email protected]>
Signed-off-by: Ryan Lo <[email protected]>
Signed-off-by: Ryan Lo <[email protected]>
46b9e91
to
124b62c
Compare
Hi @eapolinario |
Signed-off-by: Ryan Lo <[email protected]>
Signed-off-by: Ryan Lo <[email protected]>
Signed-off-by: Ryan Lo <[email protected]>
b37fdcc
to
b6c54d8
Compare
Signed-off-by: Ryan Lo <[email protected]>
b6c54d8
to
7428c83
Compare
Signed-off-by: Ryan Lo <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4946 +/- ##
==========================================
+ Coverage 59.00% 59.02% +0.02%
==========================================
Files 645 645
Lines 55672 55726 +54
==========================================
+ Hits 32847 32895 +48
- Misses 20230 20234 +4
- Partials 2595 2597 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Ryan Lo <[email protected]>
deployment/sandbox-binary/flyte_sandbox_binary_helm_generated.yaml
Outdated
Show resolved
Hide resolved
Signed-off-by: Ryan Lo <[email protected]>
I just tried this PR on a local K8s cluster with NGINX. Regardless of setting
What other flag should be set? |
@davidmirror-ops |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
@neverett any idea why the docs test fails here with a bunch of imports? |
Signed-off-by: Eduardo Apolinario <[email protected]>
- /flyteidl.service.AdminService | ||
- /flyteidl.service.AdminService/* | ||
- /flyteidl.service.AuthMetadataService | ||
- /flyteidl.service.AuthMetadataService/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We missed a *
here. Fix is https://github.com/flyteorg/flyte/pull/5185/files.
Tracking issue
Closes #4870
Why are the changes needed?
We added
.Values.ingress.separateGrpcIngress
in flyte-core chart, but not in flyte-binary.What changes were proposed in this pull request?
ingress.separateGrpcIngress
(false by default) in values.yaml for flyte-binary chartHow was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link