Skip to content
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

Closed
wants to merge 14 commits into from

Conversation

weixiao-huang
Copy link

@weixiao-huang weixiao-huang commented Aug 19, 2022

This PR fixes #30

@CLAassistant
Copy link

CLAassistant commented Aug 19, 2022

CLA assistant check
All committers have signed the CLA.

@weixiao-huang weixiao-huang force-pushed the feat/helm-deploy branch 2 times, most recently from 43eb244 to eec1a98 Compare August 20, 2022 01:02
@codecov-commenter
Copy link

codecov-commenter commented Aug 22, 2022

Codecov Report

Merging #31 (de2a44e) into main (d5ac5b2) will decrease coverage by 0.45%.
The diff coverage is n/a.

@@            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     
Impacted Files Coverage Δ
pkg/controller/vm_controller.go 21.17% <0.00%> (-1.01%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@fengye87 fengye87 requested a review from scuzhanglei August 22, 2022 02:29
@fengye87 fengye87 added the enhancement New feature or request label Aug 22, 2022
@fengye87 fengye87 changed the title feat(deploy): add virtink helm chart feat(deploy): Add Helm chart Aug 22, 2022
@scuzhanglei
Copy link
Contributor

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 .

@weixiao-huang
Copy link
Author

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

@fengye87
Copy link
Contributor

@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 :)

@weixiao-huang weixiao-huang force-pushed the feat/helm-deploy branch 2 times, most recently from 81be562 to 36acf4a Compare August 31, 2022 14:43
@weixiao-huang
Copy link
Author

I finished it with skaffold

Copy link
Contributor

@scuzhanglei scuzhanglei left a 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.

deploy/helm/virtink/Chart.yaml Outdated Show resolved Hide resolved
deploy/helm/virtink/Chart.yaml Outdated Show resolved Hide resolved
deploy/helm/virtink/Chart.yaml Outdated Show resolved Hide resolved
deploy/helm/virtink/Chart.yaml Outdated Show resolved Hide resolved
deploy/helm/virtink/values.yaml Outdated Show resolved Hide resolved
deploy/helm/virtink/values.yaml Outdated Show resolved Hide resolved
deploy/helm/virtink/values.yaml Outdated Show resolved Hide resolved
deploy/helm/virtink/values.yaml Outdated Show resolved Hide resolved
@weixiao-huang weixiao-huang force-pushed the feat/helm-deploy branch 2 times, most recently from 414486e to 46bfe31 Compare September 1, 2022 14:18
@weixiao-huang
Copy link
Author

All discussions have been resolved

@fengye87 fengye87 requested a review from scuzhanglei September 5, 2022 02:45
@weixiao-huang
Copy link
Author

Is there any problem of this PR?

@scuzhanglei
Copy link
Contributor

Hi, @weixiao-huang , sorry for the delay, there are some pending comments above need to be addressed. we need make it be compatible with skaffold too.

@weixiao-huang
Copy link
Author

could you please tell me which comments? It seems all comments have been resolved

@anisimovdk
Copy link

PR is stale? It would be nice to have helm chart to deploy.

@weixiao-huang
Copy link
Author

weixiao-huang commented Jul 16, 2023

there are some pending comments above need to be addressed. we need make it be compatible with skaffold too.

It seems all comments have been resolved, this MR should be reviewed by the maintainer

@scuzhanglei
Copy link
Contributor

there are some pending comments above need to be addressed. we need make it be compatible with skaffold too.

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.

@anisimovdk
Copy link

While waiting for this PR...
I wrote a simple chart generator script from the manifest URLs.
It helps with installing resources from plain manifests via helmfile.
Maybe this will be useful for someone who also waiting for this PR.
https://github.com/anisimovdk/chartgen

@fengye87
Copy link
Contributor

I'm thinking to generate a Helm chart by running helmify over virtink.yaml. Any thoughts? @anisimovdk @weixiao-huang

@anisimovdk
Copy link

Helmify has some limitations. I have created an issue, but it has not received any answer yet.

@fengye87
Copy link
Contributor

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 metadata.name is correct for CRDs in my generated helm chart. Actually, I cannot find any trace of RELEASE_NAME string in the generated directory. Am I missing something?

@anisimovdk
Copy link

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.

@fengye87
Copy link
Contributor

fengye87 commented Sep 19, 2023

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

@fengye87
Copy link
Contributor

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

@fengye87 fengye87 closed this Sep 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a helm chart for installing virtink
7 participants