Add support for deployment with helm #85
Conversation
There was a problem hiding this comment.
Hi Yujunz,
Firstly, thanks a lot for sending this over! It's really great that you're getting involved in enhancing the project to use Helm. Should be able to work this out and get it in soon, and then we can see what else we can add.
I've put a few specific points in line, but i think this would be a lot nicer if we can merge the function you're adding in with the existing k8s-gencfg script, and moving things around to be a bit clearer. I think the right approach would be doing the following:
- creating a new
helm/directory under thekubernetes/directory - Moving the Chart.yaml and values.yaml to under
helm/ - Parameterising the values.yaml file, similar to what is in our current templates
- Enhancing the
k8s-gencfgscript with a new function/logic that:- creates the
clearwater/helm package directory - Moves the files in
helm/across, filling in the parameterised image path and tag values (as we do for the existing templates) - Copies the main clearwater yaml files across to
clearwater/
- creates the
- add the whole
clearwater/directory to the gitignore
This would mean users should only need to run the single config generation command, and we only have one source of truth for the image path and tag, not having values hardcoded in the values.yaml.
What do you think about this approach?
kubernetes/clearwater/values.yaml
Outdated
| # This is a YAML-formatted file. | ||
| # Declare variables to be passed into your templates. | ||
| image: | ||
| path: quay.io/clearwater_ims |
There was a problem hiding this comment.
I believe this is currently pointing to your own build of Clearwater. Would be much better as a parameter that the config generation script can fill in.
kubernetes/clearwater/values.yaml
Outdated
| image: | ||
| path: quay.io/clearwater_ims | ||
| tag: autobuild | ||
| hss: |
There was a problem hiding this comment.
I can't see this being used anywhere. I may be misunderstanding how helm works, as we have relatively little experience with it. Can you explain what purpose this entry in the values.yaml has?
There was a problem hiding this comment.
It was used for creating config map. It seems that I forgot to add the template.
kubernetes/clearwater/.helmignore
Outdated
| @@ -0,0 +1,21 @@ | |||
| # Patterns to ignore when building packages. | |||
There was a problem hiding this comment.
Is this the same file taken from the example helm package?
I don't believe we will need most of this, and probably don't need any of these entries. Do you agree?
If so i think we should just remove this file
README.md
Outdated
|
|
||
| To create a helm chart, you should simply run `./k8s-genchart`. | ||
|
|
||
| If you want to use your own docker registry, modify `clearwater/values.yaml`. |
There was a problem hiding this comment.
If you're able to make the changes to build Helm config using the k8s-gencfg script, we won't need this :)
I'm assuming users will still have had to the up the config-map, as in the steps above? Haven't got helm installed on the rig i have available so haven't been able to test myself
There was a problem hiding this comment.
We may include the ConfigMap with default values in helm chart. It is quite convenient to override the default values from command arguments.
|
+1 for a single command and one source of truth. I will update the patch according to your comments :-) |
yujunz
force-pushed
the
helm
branch
2 times, most recently
from
b10547e
to
f9b05b4
Compare
|
Ready for review @johadalin Tested in GKE. |
|
@yujunz this looks great. All fits nicely into the Given it all works, I'm going to merge this down. Do feel free to send over any more enhancements if you make them in future; if one of us doesn't pick it up that quickly, tag me/someone else on the project, or assign it to one of us if you can (not sure on the repo permissions here. I'll check into it) |
Thanks for the hint, @johadalin. It seems I don't have permission to make assignments. |
|
Cool. I'll see if it's possible to change that, and if not, just @ tag us :) |
No description provided.