-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Run dashboard in a dedicated deployment with helm chart #18093
Conversation
c5273ec
to
767139d
Compare
❌ E2E Happy path tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) ℹ️ |
❌ E2E Happy path tests failed ❗ See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) ℹ️ |
9eed3f7
to
9817965
Compare
✅ E2E Happy path tests succeed 🎉 See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) |
✅ E2E Happy path tests succeed 🎉 See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) |
http: | ||
routers: | ||
che-dashboard: | ||
rule: "PathPrefix(`/dashboard/`)" |
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.
does it work without end slash?
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.
Checking, I assume that if we miss end slash, then we'll get broken redirects, like /dashboardanythings -> /dashboard since it's currently done by Che Server.
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 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.
Added end slash to ingress http path as well.
What about put dashboard yamls to custom-charts? We would avoid the need of |
I thought about it and decided not to move it there since the current custom chart is used for a bit independent components that can be optional if external is configured. I don't see a good case where dashboard can be deployed independently from Che Server, as well as Che Server without Dashboard. It's a kind of cyclic dependency. |
@sleshchenko not a strong opinion about that. I just think it would make things cleaner, not mixing 2 "independent" deployments in one folder. One usecase I can think of now is swap between dashboard and dashboard-next, where you could easily control with simple boolean values what to deploy/use. if you think it does not make sense, I'm ok with current state as well. |
✅ E2E Happy path tests succeed 🎉 See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) |
73c0b37
to
2fa1570
Compare
Good example, but we are not going to support different deployments for Dashboard and Dashboard Next. They are going to be run in one deployment. It's declared in #17647 (comment) |
✅ E2E Happy path tests succeed 🎉 See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) |
Another example might be to be able to deploy che-server only for tests. But we're not doing that atm... As I've said, I'm not against the current PR. If you think it's better as is, let's do it. |
✅ E2E Happy path tests succeed 🎉 See Details
Tested with Eclipse Che Multiuser User on K8S (minikube v1.1.1) |
What does this PR do?
Run dashboard in a dedicated deployment.
Note that after this PR Che Server tomcat will continue serving dashboard resources as well due backward compatibility reason. Tomcat will stop serving dashboard resources only after two sprints after we move helm + che operator to new deployment way.
Screenshot/screencast of this PR
What issues does this PR fix or reference?
It fixes #18076
How to test this PR?
I checked that Dashboard and Dashboard Next are accessible and work as expected in different combination Singlehost/Multihost/DefaultHost/Gateway + Single/Multi-user
chectl server:start --platform=minikube --installer=helm && helm delete che
)Singlehost
Singlehost Single-User Che
Singlehost Mutli-User Che
SingleHost Gateway
SingleHost Gateway Single-User Che
SingleHost Gateway Mutli-User Che
Multi Host
MultiHost Single-User Che
MultiHost Mutli-User Che
Default Host
Note: it's without TLS, seems that does not work with TLS properly.
Default Host Single-User Che
Default Host Mutli-User Che
PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or reference
andHow to test this PR
completedReviewers
Reviewers, please comment how you tested the PR when approving it.