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

Allow deeply nested property definition for Helm properties #4511

Merged
merged 3 commits into from
Jul 22, 2020

Conversation

gsquared94
Copy link
Contributor

@gsquared94 gsquared94 commented Jul 21, 2020

Fixes: #500
Description

Being unable to define verbose nested properties in certain helm sections of the skaffold yaml file has been a cause of user confusion since it is inconsistent with other sections that allow it. Several evidences in issue #500

This PR allows defining helm properties like:

artifactOverrides:
    image: skaffold-helm
    skaffold-helm-subchart:
        image: skaffold-helm
overrides:
   some:
       key: someValue
setValues:
   some:
       key: someValue

which makes it consistent across artifactOverrides, overrides and setValues.

The old way would have been:

artifactOverrides:
    image: skaffold-helm
    skaffold-helm-subchart.image: skaffold-helm
overrides:
   some:
       key: someValue
setValues:
   some.key: someValue

The change is backward compatible so it doesn't need any config update function.

…ifactOverrides`, `SetValues` and `SetValueTemplates`
@gsquared94 gsquared94 requested a review from a team as a code owner July 21, 2020 17:10
@gsquared94 gsquared94 requested a review from MarlonGamez July 21, 2020 17:10
@gsquared94 gsquared94 changed the title Allow deeply nested property definition for Helm properties like ArtifactOverrides, SetValues and SetValueTemplates Allow deeply nested property definition for Helm properties Jul 21, 2020
@codecov
Copy link

codecov bot commented Jul 21, 2020

Codecov Report

Merging #4511 into master will decrease coverage by 0.01%.
The diff coverage is 70.37%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4511      +/-   ##
==========================================
- Coverage   72.37%   72.36%   -0.02%     
==========================================
  Files         333      333              
  Lines       12959    12983      +24     
==========================================
+ Hits         9379     9395      +16     
- Misses       2983     2988       +5     
- Partials      597      600       +3     
Impacted Files Coverage Δ
pkg/skaffold/schema/latest/config.go 100.00% <ø> (ø)
pkg/skaffold/schema/util/util.go 76.66% <70.37%> (-5.16%) ⬇️
pkg/skaffold/update/update.go 53.57% <0.00%> (-5.81%) ⬇️
cmd/skaffold/app/cmd/flags.go 86.36% <0.00%> (ø)
cmd/skaffold/app/cmd/cmd.go 66.42% <0.00%> (+0.24%) ⬆️

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 626462e...d016d9d. Read the comment docs.

@gsquared94 gsquared94 requested review from nkubala and removed request for briandealwis, tejal29 and MarlonGamez July 22, 2020 18:34
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.

one nit but this LGTM, tried it out and it works nicely

pkg/skaffold/schema/util/util.go Outdated Show resolved Hide resolved
@gsquared94 gsquared94 merged commit 1110eb1 into GoogleContainerTools:master Jul 22, 2020
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.

helm values support nested structure
3 participants