-
Notifications
You must be signed in to change notification settings - Fork 42
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
feat(deploy): Add Helm chart #31
Conversation
43eb244
to
eec1a98
Compare
Codecov Report
@@ Coverage Diff @@
## main #31 +/- ##
==========================================
- Coverage 32.13% 31.67% -0.46%
==========================================
Files 5 5
Lines 1307 1307
==========================================
- Hits 420 414 -6
- Misses 827 831 +4
- Partials 60 62 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
thanks for you PR. as this PR copys some manifest yamls, it is not easy to maintain two kinds of manifests(kustomize and helm). maybe use skaffold with helm deploy is a better choice, so that we can remove kustomize templates and only use helm chart. thanks again . |
Sorry for my delay. I'm not familiar with skaffold, so it may takes some time for me to learn it. Please wait some moment |
@weixiao-huang Don't worry, take your time. We actually talked about merging this PR first and then we make the skaffold-helm integrate ourselves. But since skaffold is really a handy tool while developing on K8s, so I'd recommend you take this as an opportunity to equip yourself with skaffold. I'm sure it will well worth your time getting familiar with skaffold, since it can really ease your dev journey on k8s :) |
81be562
to
36acf4a
Compare
36acf4a
to
93914d8
Compare
e7ad9d4
to
e7424cb
Compare
I finished it with skaffold |
1c1f1c1
to
978aa2f
Compare
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.
thanks for your updates, I have some comments to discuss.
414486e
to
46bfe31
Compare
46bfe31
to
de2a44e
Compare
f0be22e
to
fd306d6
Compare
All discussions have been resolved |
Is there any problem of this PR? |
Hi, @weixiao-huang , sorry for the delay, there are some pending comments above need to be addressed. we need make it be compatible with |
could you please tell me which comments? It seems all comments have been resolved |
PR is stale? It would be nice to have helm chart to deploy. |
It seems all comments have been resolved, this MR should be reviewed by the maintainer |
@weixiao-huang thanks, I replied above, and plz rebase this PR, then I will review and test it again. |
While waiting for this PR... |
I'm thinking to generate a Helm chart by running helmify over virtink.yaml. Any thoughts? @anisimovdk @weixiao-huang |
Helmify has some limitations. I have created an issue, but it has not received any answer yet. |
I'm not sure if I understand you correctly, but |
I've compared the virtink plain manifest with the templated manifest generated by helmify, and it looks okay. However, helmify adds the name of the release to all 'metadata.name' fields. For example, "virtink-controller" becomes "test-release-virtinkink-controller". If this is acceptable for virtink, it can be used. |
I believe that's acceptable. I'll put some tests on the generated chart. If all goes well, I shall make it available to public |
I've published a helm chart at https://github.com/smartxworks/virtink-charts , PRs welcome. I'm closing this issue, feel free to reopen if necessary |
This PR fixes #30