-
Notifications
You must be signed in to change notification settings - Fork 13
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
✨ Update helm chart and docs to allow for the installation of CP, PCH, statusaddon #222
Conversation
Signed-off-by: francostellari <[email protected]> CP, PCH, statusaddon, docs Signed-off-by: francostellari <[email protected]> CP, PCH, statusaddon, docs Signed-off-by: francostellari <[email protected]>
014deef
to
8bc686b
Compare
scripts/create-kind-cluster.sh
Outdated
protocol: TCP | ||
EOF | ||
|
||
echo Patching nginx ingress with SSL passthrough... |
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.
The command below is a create rather than a patch, right?
scripts/create-kind-cluster.sh
Outdated
|
||
echo Patching nginx ingress with SSL passthrough... | ||
|
||
kubectl create -f https://raw.githubusercontent.com/kubestellar/kubestellar/main/example/kind-nginx-ingress-with-SSL-passthrough.yaml |
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 reference is not pinned to a particular version of the referenced content. That makes the behavior of a release of KubeFlex variable, depending on the evolution of something else.
Dependency cycles between repos are bad.
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.
easy to fix
This is creating several references from the kubeflex repo to the kubestellar repo. Yes, some are commented out, but: (a) they are still there and (b) others are not. Dependency cycles between repos are problematic. Couldn't this work be put in the kubestellar repo instead? I am thinking of leaving a Helm chart in kubeflex that just installs kubeflex, and introducing a Helm chart in kubestellar that (a) has the kubeflex chart as a dependency (listed in |
Regarding dependency issues KS <--> KF. The source of the issue is due to the desire to leave all options open:
|
@ezrasilvera @MikeSpreitzer can you clarify what cycle dependency do you see? This chart was developed by testing with specific version of kubestellar and ocm. I don't expect any change in this chart once you release ks 0.21, no staggered release. There is also no reference to the kubestellar repo, no dependency. Additionally, at this moment, I don't think ks repo teaches anybody how to use kflex helm chart either. This chart is not just for kubestellar user. There may be people that what a k8s CP with ocm in it. Additinal hook support me be added when needed. Referring to a specific image registry |
Signed-off-by: francostellari <[email protected]> Change nginx patch url Signed-off-by: francostellari <[email protected]>
@francostellari I don't understand wat you mean by "A comment is not a cycle dependency." .. |
@francostellari My last comment on dependency is indeed not related to the Helm dependency . So you can mark it as done. |
My suggestion to solve the circular dependency issue that has been pointed out by @ezrasilvera and @MikeSpreitzer here is the following:
|
@pdettori , to be clear, there is nothing usable in this PR that should be retained and I can close it, right? |
@francostellari: I think that it is very valuable to have one uber-chart to install it all. Yes, my suggestion was for a new PR to be open in the main kubestellar repo, and to close this one here. Hopefully there is content that can be re-used from here in the new PR. |
Team does not think this is a good idea. |
@francostellari, @pdettori: I think that if KubeFlex is successful then we may want to resurrect this PR because it has a couple of good ideas for general use: making it easy to install CPs and PCHs. My problem with the current draft is that those general facilities are not presented as general facilities but rather things that are here to support ks/ks. If the naming and wording (including comments) were generalized to be appropriate for any larger system using kubeflex, then I think this would make sense. To be clear, it would not remove the need for the larger system to have its own Helm chart, but more of the work could be done in this chart from kubeflex. |
Summary
This PR intends to update helm chart and docs to allow for the installation of CP, PCH, statusaddon
make chart
to separate the CP and PCH crds fromoperator.yaml
tocrds/
folder to guarantee that they are installed before the remaining of the kubeflex chartocm
,kubestellar
,statusaddon
Post Create Hooksvalues.yaml
with commented examples of CPspostgresql-hook.yaml
to simplypostgresql.yaml
user.md
docs to reflect the changesAll new features are turned off by default.
Related issue(s)
Fixes #213