-
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
Flyte-core add support for ingressClassName in ingress #4805
Flyte-core add support for ingressClassName in ingress #4805
Conversation
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.
Other than the key name, it LGTM
Thanks!
charts/flyte-core/values.yaml
Outdated
@@ -424,6 +424,8 @@ common: | |||
# -- Specify your Secret (with sensitive data) or pseudo-manifest (without sensitive data). See https://github.com/godaddy/kubernetes-external-secrets | |||
secretManifest: {} | |||
ingress: | |||
# --- Sets the ingressClassName | |||
class: |
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.
I'd prefer to keep this key name as ingressClassName
. flyte-binary
uses that structure too
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.
You got it!
{{- end }} | ||
{{- with .Values.common.ingress.separateGrpcIngressAnnotations }} | ||
{{- toYaml . | nindent 4}} | ||
{{- $annotations := .Values.common.ingress.annotations | deepCopy -}} |
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.
Nice. Thank you!
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4805 +/- ##
==========================================
+ Coverage 58.98% 59.12% +0.13%
==========================================
Files 644 644
Lines 55146 52789 -2357
==========================================
- Hits 32529 31209 -1320
+ Misses 20043 19006 -1037
Partials 2574 2574
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
- ingressClassName field was added in k8s 1.18, and effectively replaces the unofficially supported annotation `ingress.class` https://kubernetes.io/docs/concepts/services-networking/ingress/#deprecated-annotation Signed-off-by: ddl-ebrown <[email protected]>
- It's possible to have no annotations set for the ingress, which ends up rendering an annotations: This can make some validators unhappy, so rewrite the rendering to only emit annotations: when the collection is not empty Signed-off-by: ddl-ebrown <[email protected]>
8b526d8
to
bae6982
Compare
* Add support for ingressClassName in ingress - ingressClassName field was added in k8s 1.18, and effectively replaces the unofficially supported annotation `ingress.class` https://kubernetes.io/docs/concepts/services-networking/ingress/#deprecated-annotation Signed-off-by: ddl-ebrown <[email protected]> * Flyte-core don't render empty ingress annotations - It's possible to have no annotations set for the ingress, which ends up rendering an annotations: This can make some validators unhappy, so rewrite the rendering to only emit annotations: when the collection is not empty Signed-off-by: ddl-ebrown <[email protected]> --------- Signed-off-by: ddl-ebrown <[email protected]> Signed-off-by: Katrina Rogan <[email protected]>
* Add support for ingressClassName in ingress - ingressClassName field was added in k8s 1.18, and effectively replaces the unofficially supported annotation `ingress.class` https://kubernetes.io/docs/concepts/services-networking/ingress/#deprecated-annotation Signed-off-by: ddl-ebrown <[email protected]> * Flyte-core don't render empty ingress annotations - It's possible to have no annotations set for the ingress, which ends up rendering an annotations: This can make some validators unhappy, so rewrite the rendering to only emit annotations: when the collection is not empty Signed-off-by: ddl-ebrown <[email protected]> --------- Signed-off-by: ddl-ebrown <[email protected]>
Tracking issue
https://github.com/flyteorg/flyte/issues/
Why are the changes needed?
What changes were proposed in this pull request?
How was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link