Add integration testing and example for skipBuildDependencies option #1368

pscarey · 2018-12-06T13:43:12Z

Currently, it is not possible to deploy a helm chart with the following structure:

Chart.yaml
values.yaml
requirements.yaml # Declares local dependency charts, no repository field
templates/ # Chart templates
charts/
    subchart/
        Chart.yaml
        values.yaml
        templates/

The helm dep build call breaks on charts following this structure, despite the chart being valid for deployment with helm install and helm upgrade.

A key production example of this is the istio helm chart, found here.

This pull request fixes this issue with a skipDependencyBuild option for helm releases (which skips the helm dep build phase) and includes documentation, an example (based off the existing helm example) and adds a new test for the validity of the skipDependencyBuild option.

codecov-io · 2018-12-06T13:50:09Z

Codecov Report

Merging #1368 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1368   +/-   ##
=======================================
  Coverage   47.91%   47.91%           
=======================================
  Files         144      144           
  Lines        6499     6499           
=======================================
  Hits         3114     3114           
  Misses       3101     3101           
  Partials      284      284

Impacted Files	Coverage Δ
pkg/skaffold/deploy/helm.go	`61.92% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2e4c4a9...b2089a2. Read the comment docs.

balopat

Thank you for the contribution, can you change the examples only under the integration folder? The examples in the root folder are for the latest released version for compatibility reasons.

integration/examples/helm-deployment-dependencies/skaffold.yaml

…dependencies/skaffold.yaml Co-Authored-By: pscarey <pscarey@users.noreply.github.com>

nkubala · 2019-01-07T17:47:35Z

once you rebase the docs LGTM

…dependencies/skaffold.yaml Co-Authored-By: pscarey <pscarey@users.noreply.github.com>

googlebot · 2019-01-14T03:03:35Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

nkubala · 2019-02-01T21:33:09Z

sorry, looks like you need one more rebase 😨 send a ping once you do and we'll get this merged so you don't have to keep fighting with it. also, once you get #1371 ping us there as well so we can get that reviewed/merged ASAP

balopat · 2019-03-08T19:56:12Z

pkg/skaffold/deploy/helm.go

@@ -161,7 +161,7 @@ func (h *HelmDeployer) deployRelease(ctx context.Context, out io.Writer, r lates
 	// Dependency builds should be skipped when trying to install a chart
 	// with local dependencies in the chart folder, e.g. the istio helm chart.
 	// This decision is left to the user.
-	if !r.SkipBuildDependencies {


we should do renames only if it's really necessary - I feel like the benefit here is marginal - even though I understand where you're coming from. Do you mind reverting this?

balopat · 2019-03-08T20:22:12Z

In order to merge this in before next release (next week) we need:

rebase to latest
remove the v1beta4 changes
revert the rename

I'm happy to add this @pscarey if you are okay with that.

pscarey · 2019-03-09T01:00:06Z

Thanks @balopat

I've updated the branch to the latest master and removed the v1beta4 change, and updated the schemas for tests to pass.

The rename was actually fixing a mistake when I reverted the name to my first draft. The original comment in this PR refers to the option as 'skipDependencyBuild'. In commit 2acb227 I accidentally rolled this back to my first draft of 'skipBuildDependencies', so I corrected it back to 'skipDependencyBuild' in 4e23a49.

Please let me know if there's anything else you require. Keen to start using this shortly at work, pending the other PR. Hopefully that can be simpler.

eraac · 2019-03-09T17:10:49Z

Please let me know if there's anything else you require. Keen to start using this shortly at work, pending the other PR. Hopefully that can be simpler.

You can already use an option for skip dependencies since #1254 was merged and release with 0.22 (https://github.com/GoogleContainerTools/skaffold/releases/tag/v0.22.0)

pscarey · 2019-03-09T23:32:10Z

Ah, that's where the naming confusion has come from. This PR then only includes an integration example over that (+ currently, the re-name). Please let me know if you want the PR re-named + integration example to be merged, or alternatively just close the PR.

R.E. usage, we need #1371, which I can now update off the current master & can be looked at again.

balopat · 2019-03-14T21:37:34Z

There is value in the integration example - but please revert the rename - I think that the value there is outweighed by the noise it creates during upgrades.

pscarey · 2019-03-20T08:29:51Z

Rename has been reverted. Related PR has been separated out from this to work against the current master.

Only other change made was that to pass tests, the integrations example had to also be added to the root examples folder, contrary to the previous request by @balopat

Thank you for the contribution, can you change the examples only under the integration folder? The examples in the root folder are for the latest released version for compatibility reasons.

pscarey added 4 commits December 6, 2018 23:33

Added skipDependencyBuild option to Helm deployment

d8c7447

Added example of using helm with skipBuildDependencies

21147f1

Updated annotated-skaffold.yaml

542dcb7

Example + docs fixes

b6f4a34

pscarey requested review from balopat, dgageot, nkubala and r2d4 as code owners December 6, 2018 13:43

pscarey changed the title ~~Add option to skip helm dependency build phase~~ Add option to skip helm dependency build phase fixes #1346 Dec 7, 2018

pscarey changed the title ~~Add option to skip helm dependency build phase fixes #1346~~ Add option to skip helm dependency build phase Dec 7, 2018

balopat added the kokoro:run runs the kokoro jobs on a PR label Dec 7, 2018

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Dec 7, 2018

balopat suggested changes Dec 7, 2018

View reviewed changes

balopat added the !! config-change !! label Dec 7, 2018

pscarey mentioned this pull request Dec 8, 2018

Automatically watch helm subcharts when skipBuildDependencies is enabled #1371

Merged

pscarey added 2 commits December 8, 2018 13:18

Move example + annotated-skaffold.yaml to integration folder

8a7816e

Corrected README in examples, updated integration examples README

f0730f7

balopat reviewed Dec 8, 2018

View reviewed changes

integration/examples/helm-deployment-dependencies/skaffold.yaml Outdated Show resolved Hide resolved

Update apiVersion to v1beta2 in integration/examples/helm-deployment-…

25d9e31

…dependencies/skaffold.yaml Co-Authored-By: pscarey <pscarey@users.noreply.github.com>

balopat added the kokoro:run runs the kokoro jobs on a PR label Dec 10, 2018

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Dec 10, 2018

nkubala approved these changes Jan 7, 2019

View reviewed changes

pscarey and others added 5 commits January 14, 2019 13:59

Added skipDependencyBuild option to Helm deployment

68aa15f

Updated annotated-skaffold.yaml

d210c26

Move example + annotated-skaffold.yaml to integration folder

53a9865

Update apiVersion to v1beta2 in integration/examples/helm-deployment-…

7ea92d8

…dependencies/skaffold.yaml Co-Authored-By: pscarey <pscarey@users.noreply.github.com>

Merge deleted README.adoc

2a0cae1

googlebot added the cla: yes label Jan 29, 2019

balopat added 3 commits January 28, 2019 16:52

revert rushed change

7596048

upgrade to v1beta4

8876a51

remove unnecessary header from example doc

831515f

balopat added the kokoro:run runs the kokoro jobs on a PR label Jan 30, 2019

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jan 30, 2019

dgageot added the needs-rebase label Jan 31, 2019

Merge upstream into branch

2acb227

pscarey requested a review from priyawadhwa as a code owner February 22, 2019 00:12

pscarey added 2 commits February 22, 2019 11:18

Fix naming errors - change option to SkipDependencyBuild

4e23a49

Update schemas

010dabb

balopat reviewed Mar 8, 2019

View reviewed changes

pscarey added 3 commits March 9, 2019 11:43

Merge master

ffd87ae

Remove v1beta4 change

0c3202b

Update schemas

de81c9f

balopat removed the !! config-change !! label Mar 14, 2019

Revent rename

14d24f0

pscarey changed the title ~~Add option to skip helm dependency build phase~~ Add integration testing and example for skipBuildDependencies option Mar 20, 2019

pscarey added 2 commits March 20, 2019 18:13

Merge upstream

7c1e72e

Schema, docs, examples updates

b2089a2

tejal29 added the kokoro:run runs the kokoro jobs on a PR label Mar 20, 2019

kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Mar 20, 2019

nkubala merged commit 7b054a1 into GoogleContainerTools:master Mar 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add integration testing and example for skipBuildDependencies option #1368

Add integration testing and example for skipBuildDependencies option #1368

pscarey commented Dec 6, 2018 •

edited

Loading

codecov-io commented Dec 6, 2018 •

edited

Loading

balopat left a comment

nkubala commented Jan 7, 2019

googlebot commented Jan 14, 2019

nkubala commented Feb 1, 2019

balopat Mar 8, 2019

balopat commented Mar 8, 2019

pscarey commented Mar 9, 2019

eraac commented Mar 9, 2019

pscarey commented Mar 9, 2019

balopat commented Mar 14, 2019

pscarey commented Mar 20, 2019

Add integration testing and example for skipBuildDependencies option #1368

Add integration testing and example for skipBuildDependencies option #1368

Conversation

pscarey commented Dec 6, 2018 • edited Loading

codecov-io commented Dec 6, 2018 • edited Loading

Codecov Report

balopat left a comment

Choose a reason for hiding this comment

nkubala commented Jan 7, 2019

googlebot commented Jan 14, 2019

nkubala commented Feb 1, 2019

balopat Mar 8, 2019

Choose a reason for hiding this comment

balopat commented Mar 8, 2019

pscarey commented Mar 9, 2019

eraac commented Mar 9, 2019

pscarey commented Mar 9, 2019

balopat commented Mar 14, 2019

pscarey commented Mar 20, 2019

pscarey commented Dec 6, 2018 •

edited

Loading

codecov-io commented Dec 6, 2018 •

edited

Loading