-
-
Notifications
You must be signed in to change notification settings - Fork 78
Initial impl of helm chart #2037
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
Initial impl of helm chart #2037
Conversation
@lvca I never used helm charts, so I don't think I can be of help here (at least in the short run). Sorry. |
readinessProbe: | ||
httpGet: | ||
path: / | ||
port: http |
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's an endpoint /api/v1/ready
that return 204 if all is ok. It could be usefull?
Great works, thanks. The dedicated repo approach works particularly well because:
Let me think a bit about the better way to manage this. BTW, a great and valuable work, thank you! |
After some consultation, the PR will be merged here, instead of creating a new, dedicated repo |
Just a small request: can you add a readme.md with some basic instructions on how to use it? moreover, What should we do on relases? Just upgrade the version inside the files? |
Yes, I'll add a README in a bit. |
Added a README in 631367e by using Bitnami's helm readme generator: https://github.com/bitnami/readme-generator-for-helm If you like, and time opens up for me I can probably add that as a step to the helm test perhaps, install that doc generator and run it using the Aside from that, could also do a minikube step to install the chart using default values and just make sure some pods/svc end up happy. But again, I'm a bit slim for time these days but can probably swing it to be work related to a certain extent. :) |
ac171f1
to
631367e
Compare
a dedicated workflow to test helm with minikube and also the doc would be amazing. Let me know when I can merge, thanks |
I think we're aligned here :) As mentioned we're intending to use this for a PoC project so will be sure to upstream anything we see during our use. Thanks, ready to merge from my end. |
Thanks @milesgranger! @gramian the readme.md provided by @milesgranger should be also in our docs. WDYT? |
I suggest to add a section, as for Docker and plain K8s, to the docs with some basic info about helm and a reference to this chart. I would rather not add the parameters as this is very specific to this very chart, and rather refer to the README instead. Also from me, thanks @milesgranger ! BTW: I guess, the liveness probe should not be the ready endpoint, but likely user specific ie a |
What does this PR do?
This adds an initial Helm chart at
k8s/helm
(maybe a different path is preferred?)100% organic human generated code; I used
helm create
and tried to follow the recommended best practices. However I doubt it's bug free, in fact, I think something is wrong with theingress
portion. At least when I use Traefik it seems to keep getting routed to the headless service, resulting inno available server
from Traefik. I think this has something to do with the selectors getting mixed up, but I'm not sure. I plan to poke at it a bit more today but unless I find quick success, I see adding a separateIngressRoute
directly works w/o issue so may just use that in the shorter term.Otherwise, I think it replicates pretty closely to what the existing static manifests accomplish and seems to work nicely on my end. I'm happy to continue to add to this as we use it more.
Motivation
We have a standard flow of using helm with ArgoCD for base services and would like to give ArcadeDB a go in the same way.
Related issues
#1465 (comment)
Checklist
mvn clean package
command