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

Automatically watch helm subcharts when skipBuildDependencies is enabled #1371

Merged
merged 16 commits into from
May 7, 2019
Merged

Automatically watch helm subcharts when skipBuildDependencies is enabled #1371

merged 16 commits into from
May 7, 2019

Conversation

pscarey
Copy link
Contributor

@pscarey pscarey commented Dec 7, 2018

Fixes #1347

Previously, the Helm deployer was set to ignore chart dependencies, so updating a sub chart didn't trigger a re-deploy in when running skaffold dev. This pull request removes that restriction, so that chart dependencies are included in the watch list.

@codecov-io
Copy link

codecov-io commented Dec 7, 2018

Codecov Report

Merging #1371 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1371      +/-   ##
==========================================
+ Coverage   55.96%   56.03%   +0.07%     
==========================================
  Files         173      175       +2     
  Lines        7566     7616      +50     
==========================================
+ Hits         4234     4268      +34     
- Misses       2928     2940      +12     
- Partials      404      408       +4
Impacted Files Coverage Δ
pkg/skaffold/deploy/helm.go 62.55% <100%> (+0.15%) ⬆️
pkg/skaffold/schema/validation/validation.go 100% <0%> (ø) ⬆️
pkg/skaffold/schema/versions.go 74.35% <0%> (ø) ⬆️
pkg/skaffold/build/kaniko/types.go
pkg/skaffold/build/kaniko/sources/localdir.go
pkg/skaffold/build/kaniko/sources/gcs.go
pkg/skaffold/build/kaniko/run.go
pkg/skaffold/build/kaniko/sources/sources.go
pkg/skaffold/build/kaniko/kaniko.go
pkg/skaffold/build/kaniko/secret.go
... and 12 more

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 9a68f2a...9216981. Read the comment docs.

@balopat balopat added the kokoro:run runs the kokoro jobs on a PR label Dec 7, 2018
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Dec 7, 2018
Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

LGTM - I'm just going to confirm if there was an historical reason why it was not enabled...

balopat
balopat previously requested changes Dec 7, 2018
Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

@pscarey this is not going to be that easy seemingly.
#932
We run helm dep build on rebuilds that causes an infinite loop. Did you not run into this issue?

@pscarey
Copy link
Contributor Author

pscarey commented Dec 8, 2018

Thanks for raising that issue.

I ran the existing go tests with only these changes, but am current successfully using #1368 in conjunction with this on our actual helm charts (as #1368 is required to run our charts).

Would you be happy if I paused this until we've completed the merge of #1368, then only allowed automatic watching of subcharts when the Helm skipDependencyBuild option is enabled? I will also document this caveat.

@dgageot dgageot added the kokoro:run runs the kokoro jobs on a PR label Jan 28, 2019
@kokoro-team kokoro-team removed the kokoro:run runs the kokoro jobs on a PR label Jan 28, 2019
@pscarey pscarey requested a review from priyawadhwa as a code owner March 20, 2019 07:58
@pscarey pscarey changed the title Helm watch subcharts Automatically watch helm subcharts when skipBuildDependencies is enabled Mar 20, 2019
@pscarey
Copy link
Contributor Author

pscarey commented Mar 20, 2019

The 'helm dep build' command modifies files in the charts directory of the main helm chart. By default any files in that directory are excluded from the watch list, but if skipBuildDependencies is enabled those files are now automatically included.

@nkubala
Copy link
Contributor

nkubala commented Apr 25, 2019

I think this will work now that #1368 is in, @pscarey can you rebase please?

@pscarey pscarey requested a review from tejal29 as a code owner April 29, 2019 00:16
@pscarey
Copy link
Contributor Author

pscarey commented Apr 29, 2019

@nkubala Have updated.

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.

@pscarey thanks for updating this, it looks good. would you mind adding a small section to the docs to briefly explain this? just want to make sure this doesn't get buried 👍

@pscarey
Copy link
Contributor Author

pscarey commented Apr 30, 2019

@nkubala Hope the description is clear. Please add the docs-modifications label. Thanks.

@nkubala nkubala added the docs-modifications runs the docs preview service on the given PR label May 7, 2019
@nkubala nkubala dismissed balopat’s stale review May 7, 2019 22:10

addressed

@nkubala nkubala merged commit ceda301 into GoogleContainerTools:master May 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes deploy/helm docs-modifications runs the docs preview service on the given PR needs-rebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Doesn't detect changes to subcharts
7 participants