-
Notifications
You must be signed in to change notification settings - Fork 46
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
[opsportal] Cleanup kommander/opsportal charts #376
Conversation
@@ -1,52 +1,3 @@ | |||
kommander-ui: |
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.
this part is replaced with values.yaml in kommander repo: https://github.com/mesosphere/kommander/blob/master/chart/kommander-ui/values.yaml
# Konvoy UI | ||
############################################################################### | ||
kommander: | ||
kommander-ui: |
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.
removing/replacing values from full fledged kommander to only kommander ui.
148cbc0
to
6ec2a3d
Compare
@alejandroEsc @jimmidyson @joejulian @hectorj2f I've started again with splitting up our charts so that we have opsportal only containing konvoy related stuff and only depending on UI, not whole of kommander. would appreciate some eyes on this, checking if I missed something. 🙇 |
also cc @samba |
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.
Looks good so far, can we get some deployment output?
opsportal worked fine, heres the output:
kommander caused some trouble which doesnt seem to be related, but that I'd like to understand… will post an update once I know whats going on ( edit: ref on kommander issue: https://mesosphere.slack.com/archives/CJQURP1FT/p1579780553002300 |
this has 2 approvals already, @makkes are your questions answered? would like to merge :) |
I'd prefer us to test the chart before merging. On it. |
started konvoy cluster with kommander disabled and manually installed afterwards, worked
|
|
5d085f7
940f94e
to
5d085f7
Compare
Steps to reproduce the issue:
I also tried killing opsportal-kommander-ui container (maybe it refreshed on start?) but that didnt help: @brandonc I think you have some context around capability checking, if you got time today, can you have a look please? |
this commit cleans up kommander and opsportal charts in a way that we only have kommander related addons in kommander chart and ops portal only consists of what is included in konvoy.
71916f7
to
7404172
Compare
7404172
to
d511159
Compare
apperantly its not enough to install kommander via helm, it has to be the kubeaddon, that is installed. so this PR should be good to go 🤷♂ |
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 understand that we will live with this chart not being installable standalone. If that's correct, then I'll +1. @juliangieseke can you confirm?
both charts, opsportal and kommander, should and do work standalone. my comment was related to UI feature detection which looks for the kommander clusteraddon being installed. |
…l-charts [opsportal] Cleanup kommander/opsportal charts
this commit cleans up kommander and opsportal charts in a way that we only have kommander related addons in kommander chart and ops portal only consists of what is included in konvoy.
see comments in changed code fore more info
next step will be to actually remove UI parts from both charts and make it its own kubeaddon