-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
[Helm] update ingress templates #7132
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #7132 +/- ##
===========================================
- Coverage 81.44% 81.43% -0.02%
===========================================
Files 364 364
Lines 39904 39904
Branches 3699 3699
===========================================
- Hits 32501 32495 -6
- Misses 7403 7409 +6
|
Not sure what you mean by this - isn't it already possible to set any value using an override file? |
traefik.ingress.kubernetes.io/router.entrypoints: web | ||
kubernetes.io/ingress.class: traefik |
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 removed these annotations from here, but you didn't re-add them anywhere else. Is the user supposed to set them manually when enabling Traefik?
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.
There is no need to add the first line because this is the default behavior
The second line is deprecated, ingressclass is now used instead
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.
There is no need to add the first line because this is the default behavior
Okay.
The second line is deprecated, ingressclass is now used instead
Indeed, but ingress.className
defaults to blank. Shouldn't it default to traefik
when the traefik
setting is true?
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.
done
Co-authored-by: Roman Donchenko <roman@cvat.ai>
Co-authored-by: Roman Donchenko <roman@cvat.ai>
Co-authored-by: Roman Donchenko <roman@cvat.ai>
Co-authored-by: Roman Donchenko <roman@cvat.ai>
… traefik.enabled is set to true
…ed (#7184) This PR is complementary to #7132. #7132 updated ingress templates to work with `traefik.enabled=false` and `ingress.enabled=true` values. However the templates in `templates/analytics/middlewares` path lead to creation of resources of kind `Middleware`, which is a Traefik CRD. Helm install fails if Traefik is not installed on the cluster. Changing the top-level conditional from `ingress.enabled` to `traefik.enabled` solves the issue.
…ed (cvat-ai#7184) This PR is complementary to cvat-ai#7132. cvat-ai#7132 updated ingress templates to work with `traefik.enabled=false` and `ingress.enabled=true` values. However the templates in `templates/analytics/middlewares` path lead to creation of resources of kind `Middleware`, which is a Traefik CRD. Helm install fails if Traefik is not installed on the cluster. Changing the top-level conditional from `ingress.enabled` to `traefik.enabled` solves the issue.
Added:
ingress: true/false
Motivation and context
How has this been tested?
Checklist
develop
branch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.