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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial implementation of Helm Renderer #3929

Conversation

tete17
Copy link
Contributor

@tete17 tete17 commented Apr 9, 2020

Related:#1937

Description
This PR should add Render capabilities to the Helm deployer.
I have added support for the following feature:

  • valuesFiles
  • setValueTemplates
  • values
  • Namespaced charts

I hope this is enough, If I have missed some that are consider a must please let me know. I have also tried having a good support between Helm v3 and v2.

User facing changes (remove if N/A)

The only user facing change (except the obvious addition of skaffold render for helm, can bee seen here.

I have changed this behaviour because it was assuming all members of the values map were supposed to be images built by skaffold. This is use to make skaffold replace the images by the combination of the image:tag in the manifest.

I have modified so it only looks for the appropiate images and doesn't fail if you add something like

values: 
   image: "skaffold-helm"
   replicas: 3

This lead me to modify certain unit tests accordingly. I am not entirely sure about this change I am by no means and expert in helm so feedback is highly welcomed.

It is also my first contribution to a go project so please be gentle 馃槃. I hope I followed best practice I have a feeling I am duplicating some code so please let me know if someone else also has that feeling.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

馃摑 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

鈩癸笍 Googlers: Go here for more info.

@tete17
Copy link
Contributor Author

tete17 commented Apr 9, 2020

@googlebot I signed it!

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

鈩癸笍 Googlers: Go here for more info.

@tete17 tete17 force-pushed the Add-Render-Capabilities-to-Helm-Deployer branch from 7e2d0c3 to 572fd3d Compare April 9, 2020 12:18
@googlebot
Copy link

CLAs look good, thanks!

鈩癸笍 Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Apr 9, 2020
@codecov
Copy link

codecov bot commented Apr 9, 2020

Codecov Report

Merging #3929 into master will increase coverage by 0.02%.
The diff coverage is 77.55%.

Impacted Files Coverage 螖
pkg/skaffold/deploy/helm.go 80.27% <77.55%> (+0.02%) 猬嗭笍

@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

鈩癸笍 Googlers: Go here for more info.

@googlebot googlebot added cla: no and removed cla: yes labels Apr 9, 2020
@tete17 tete17 force-pushed the Add-Render-Capabilities-to-Helm-Deployer branch from 83f67a1 to 36b12b0 Compare April 9, 2020 13:27
@googlebot
Copy link

CLAs look good, thanks!

鈩癸笍 Googlers: Go here for more info.

@googlebot googlebot added cla: yes and removed cla: no labels Apr 9, 2020
@nkubala nkubala self-assigned this Apr 13, 2020
@abh
Copy link

abh commented Apr 14, 2020

At a glance this also fixes a bug where the namespace specified with skaffold -n ... isn't actually used. Yay @tete17 :-)

@tete17 tete17 force-pushed the Add-Render-Capabilities-to-Helm-Deployer branch from 99837b5 to 68f28bd Compare April 17, 2020 17:29
@tete17
Copy link
Contributor Author

tete17 commented Apr 17, 2020

I rebased the branch to the latest master

@tete17 tete17 requested a review from tstromberg April 17, 2020 17:33
@tete17 tete17 force-pushed the Add-Render-Capabilities-to-Helm-Deployer branch from 68f28bd to b380029 Compare April 21, 2020 15:24
@tete17
Copy link
Contributor Author

tete17 commented Apr 21, 2020

I rebased once more.

Is there anything I am missing to get a review? I don't want this to be forgotten, maybe I have done something wrong but I can't seem to find anythin on DEVELOPMENT.md nor CONTRIBUTING.md

@tete17 tete17 force-pushed the Add-Render-Capabilities-to-Helm-Deployer branch from b380029 to 59c9f62 Compare April 24, 2020 15:20
@tejal29 tejal29 added the kokoro:run runs the kokoro jobs on a PR label Apr 27, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Apr 27, 2020
Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tete17 thanks so much for the contribution, and sorry for the late review! I left a few comments but this is actually looking pretty good. it would be great to get an integration test for this as well - would you mind adding a quick test case or two to pkg/integration/render_test.go? you can model it after the existing tests there for the other renderers.

pkg/skaffold/deploy/helm.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/helm.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/helm.go Show resolved Hide resolved
pkg/skaffold/deploy/helm.go Outdated Show resolved Hide resolved
pkg/skaffold/deploy/helm.go Show resolved Hide resolved
@tete17 tete17 force-pushed the Add-Render-Capabilities-to-Helm-Deployer branch from 94464ed to 6c50014 Compare May 1, 2020 11:58
@tete17
Copy link
Contributor Author

tete17 commented May 1, 2020

@nkubala Thank you very much for the review.

I hope I didn't gave the impression of desperate 馃槅 by asking for a review.

I went ahead and I think addressed all of your comment, as well as rebasing to the latest master.

Took me a bit of time but I also added some integrations tests. Let me know if you think we can add more content to them.

@tete17 tete17 force-pushed the Add-Render-Capabilities-to-Helm-Deployer branch from 2c5d425 to aa135e4 Compare May 1, 2020 14:18
@tete17 tete17 requested a review from nkubala May 4, 2020 17:42
@balopat balopat added the kokoro:run runs the kokoro jobs on a PR label May 4, 2020
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label May 4, 2020
Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tete17 this looks great! see my comment about the change to how we process the helm values from the skaffold.yaml, I think we should move that to a separate PR. once you take that out this LGTM and I'll merge it!

@tete17 tete17 force-pushed the Add-Render-Capabilities-to-Helm-Deployer branch 2 times, most recently from a032b79 to 5996138 Compare May 7, 2020 17:40
@tete17
Copy link
Contributor Author

tete17 commented May 7, 2020

I used this chance to rebase once more and cleanup the commits to make a nice git history.

Take it from here @nkubala

@tete17 tete17 requested a review from nkubala May 7, 2020 17:56
@tete17 tete17 force-pushed the Add-Render-Capabilities-to-Helm-Deployer branch 2 times, most recently from 2f28c54 to 4ce49db Compare May 13, 2020 17:31
@tete17
Copy link
Contributor Author

tete17 commented May 15, 2020

@nkubala can you review?

pkg/skaffold/deploy/helm.go Show resolved Hide resolved
@tete17 tete17 force-pushed the Add-Render-Capabilities-to-Helm-Deployer branch from 4ce49db to 8a00464 Compare May 20, 2020 18:02
@tete17
Copy link
Contributor Author

tete17 commented May 20, 2020

I went ahead and rebased once more to fix the conflicts introduced in #4169

@tete17 tete17 requested a review from tstromberg May 20, 2020 18:05
Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks so much for all your hard work getting this one in @tete17!

@nkubala nkubala merged commit ae0d872 into GoogleContainerTools:master May 21, 2020
@tete17
Copy link
Contributor Author

tete17 commented May 22, 2020

My pleasure thanks to @nkubala & @tstromberg for the constant reviews.

I also wonder is we could use this chance to also move skaffold render out of alpha and into beta since all deployers are supported now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants