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

helm values support nested structure #500

Closed
sstarcher opened this issue May 2, 2018 · 16 comments · Fixed by #4511
Closed

helm values support nested structure #500

sstarcher opened this issue May 2, 2018 · 16 comments · Fixed by #4511
Assignees
Labels
area/deploy deploy/helm help wanted We would love to have this done, but don't have the bandwidth, need help from contributors july-chill kind/feature-request priority/p1 High impact feature/bug.

Comments

@sstarcher
Copy link

Issues without logs and details are more complicated to fix.
Please help us!

Expected behavior

Support for nested helm values.

Actual behavior

Parse error

Information

Skaffold version: 0.5.0
Operating system: osx
deploy:
  helm:
    releases:
    - name: chart
      chartPath: ./chart
      namespace: skaffold
      values:
        subchart:
          image:
            tag: "abc"

Steps to reproduce the behavior

  1. . skaffold run with a value that is more than a single nesting deep
@balopat balopat added priority/p3 agreed that this would be good to have, but no one is available at the moment. priority/awaiting-more-evidence Lowest Priority. May be useful, but there is not yet enough supporting evidence. and removed priority/p3 agreed that this would be good to have, but no one is available at the moment. labels Sep 4, 2019
@balopat
Copy link
Contributor

balopat commented Sep 4, 2019

This issue received no attention for a long time - I'm wondering if people are still interested in having nested values?

@kylewolfe
Copy link

This is needed as it is currently impossible to use skaffold with any umbrella chart, custom or not.

I have referenced this issue in another proposal that would resolve this issue as well as enable injecting the image tag into custom builds.

@TBBle
Copy link
Contributor

TBBle commented Jan 21, 2020

deploy:
  helm:
    releases:
    - name: chart
      chartPath: ./chart
      namespace: skaffold
      values:
        subchart.image.tag: "abc"

works for me.

@kylewolfe
Copy link

I originally viewed this issue as a request around being able to use templated fields in nested locations, but looking again, that's not mentioned here.

For those looking for an issue about that: #3343

@bitsofinfo
Copy link

bitsofinfo commented Feb 14, 2020

this is a deal breaker for me, I can't use my chart which needs nested values

deploy:
  helm:
    releases:
      - name: test-dog-4f205e7-dirty
        chartPath: bitsofinfo-appdeploy/appdeploy
        version: 1.1.14
        wait: true
        namespace: test-apps
        valuesFiles:
          - /my/values.yaml
        values:
          app:
            context: stage-test
            environment: stage
            name: dog

blows up with cannot unmarshal !!map into string

also it appears variables are not even supported... i.e. how can I reference image tag etc...

@TBBle
Copy link
Contributor

TBBle commented Feb 18, 2020

@bitsofinfo try:

deploy:
  helm:
    releases:
      - name: test-dog-4f205e7-dirty
        chartPath: bitsofinfo-appdeploy/appdeploy
        version: 1.1.14
        wait: true
        namespace: test-apps
        valuesFiles:
          - /my/values.yaml
        overrides:
          app.context: stage-test
          app.environment: stage
          app.name: dog

Unless those override values are used to select a Docker image, in which change change overrides back to values.

@TBBle
Copy link
Contributor

TBBle commented Feb 18, 2020

According to the docs env-vars are only definitely supported with setValueTemplates, so you might want something like:

deploy:
  helm:
    releases:
      - name: test-dog-4f205e7-dirty
        chartPath: bitsofinfo-appdeploy/appdeploy
        version: 1.1.14
        wait: true
        namespace: test-apps
        valuesFiles:
          - /my/values.yaml
        setValueTemplates:
          app.context: stage-{{.CURRENT_STAGE}}
        overrides:
          app.environment: stage
          app.name: dog

It says the list is not exhaustive though, so they might be working with values, setValues, or overrides, and if not, and there's a reason they should, a feature request would seem reasonable.

@bitsofinfo
Copy link

will give it a shot but a bit of a confusing contract.

@bitsofinfo
Copy link

@TBBle fyi you suggestion works, thanks!!!

honestl skaffold in this area is a bit confusing.

values, overrides, setValuesTemplates etc etc..... why so many diff options? And why is templated vars just not supported universally?

Overall its just confusing.

@TBBle
Copy link
Contributor

TBBle commented Feb 20, 2020

I'm not sure about why templated values aren't universal. The others however, are historical, from what I've understood.

values is specifically used to support overriding the images in the Helm charts, to point them back to the artifacts declared elsewhere in the skaffold.yaml. This is because skaffold needed some understanding of the structure of those values, in order to adjust them appropriately for imageStrategy settings other than fqn.

The documentation does not, in my opinion, make this usefully clear at all.

setValues just maps directly to --set on the helm command line.

Later, people wanted things that mapped to --values as well (presumably because they're not sticky, but it could be due to parsing differences), but the name values was already used in skaffold.yaml at this level, so they chose overrides.

I guess the advantage of distinct setValuesTemplates is that you have a more-isolated list of things that might be affected by skaffold's operating environment. Helm itself deliberately does not provide a way to use system env-vars in evaluating the values of a chart, to ensure repeatability and isolation, so this is somewhat consistent, but it's not a strong argument in my opinion.

@bitsofinfo
Copy link

what further evidence is this issue awaiting since 2018? I think its pretty common for people to expect that when you declare/write YAML that you can nest properties. The currently solution of having to define these structures as flat key.paths.x.y.z is awkward.

@nkubala nkubala added priority/p1 High impact feature/bug. help wanted We would love to have this done, but don't have the bandwidth, need help from contributors and removed priority/awaiting-more-evidence Lowest Priority. May be useful, but there is not yet enough supporting evidence. labels Feb 29, 2020
@nkubala
Copy link
Contributor

nkubala commented Feb 29, 2020

@bitsofinfo agree, that was an old label that we haven't updated in a while. I do think this would be a useful feature to have.

the origin of the setValuesTemplates was primarily for use in CI/CD systems: skaffold was a big part of the early development of jenkins x. in general we wanted to support templating within reason for helm, so we kept this around. @TBBle as far as overrides go, that's not really accurate. we added support for overrides as a way to address some limitations with how values are passed to helm through --set - you can see the explanation for this in #585.

I'd love to hear feedback on how we can at least improve the documentation to clear up confusion though! also, contributions for either of these would be greatly appreciated :) realistically, we probably won't be able to prioritize this as a team in the near future.

@TBBle
Copy link
Contributor

TBBle commented Mar 1, 2020

#585's rationale and #632's description are my information source for that origin story of overrides:

Currently, it's non-intuitive to be able to only set image related values via the values field, noted in #193.

and

When looking at the code more I saw the reason that values only contains images so I made a new field called overrides that will contain the contents of the generated override values.yaml.

respectively.

#632 was pulled in without any apparent discussion of the author's proposal to renamed values to artifacts and use values for what is now overrides.


As far as documentation feedback, https://skaffold.dev/docs/references/yaml/ describes values as

key-value pairs supplementing the Helm values file.

which doesn't in any way hint that these values are only useful for setting image tags.

The only way to tell is by trying to use values in the "intuitive" way, and hitting the failure case at

return nil, fmt.Errorf("no build present for %s", imageName)

values is doubly-non-intuitive here because it actually generates --set-string, rather than a YAML file that you'd pass to Helm's --values parameter.

Perhaps a clearer description would be

key-value pairs for imageStrategy. Maps IMAGE-NAME to an image in the artifacts list, to generate appropriate --set-string flags to pass to the Helm CLI.

That's getting off topic here though. I'll look at opening a pull request during the week to try and improve documentation of values, and see if there's any other places that could be improved, e.g., https://skaffold.dev/docs/pipeline-stages/deployers/

Edit: Never got around to that pull-request.... Also, values has been renamed artifactOverrides, which is a step in the right direction. It still behaves as I described here (using --set-string), not with structured input as implied by the documentation.

Edit again: I did the bare-minimum PR to correct the incorrect part: #4487


Then this issue can remain focused on implementing more yaml-like syntax for overrides.

I'd suggest that it's not needed for values (Renamed to artifactOverrides), setValues, or setValueTemplates, because those are all --set-wrappers, for which Helm takes the dotted-format that works now, and would be problematic to use with nested structures, e.g., if overriding an element of an array, without mangling the other elements, for which YAML has no feasible syntax.

@woodcockjosh
Copy link

woodcockjosh commented Jul 16, 2020

I used to use the "values" parameter to set the value of the image which is contained in a child chart. I do this because that child chart is a re-usable component for all my apps. Now I have no way of using skaffold. This is very disappointing.

At a minimum, I have to be able to set the image like this:

childChart:
   image: 

My child chart cannot possibly access values set in the parent chart.

I agree that I couldn't understand the difference between values and setValues but at least it worked.

---- UPDATE ----

After digging through the source code looking where I could make a PR I discovered I can do this:

skaffold.yaml

apiVersion: skaffold/v2beta5
kind: Config
build:
  local:
    push: false
  artifacts:
    - image: myImage
      context: ../
  helm:
    releases:
      - name: "mychart"
        artifactOverrides:
          childChart.image: myImage

so that I can access the image set by skaffold with {{ .Values.image }} in the child chart. Might be a good idea to update these docs:
https://skaffold.dev/docs/pipeline-stages/deployers/helm/

@TBBle
Copy link
Contributor

TBBle commented Jul 17, 2020

Yeah, looks like artifactOverrides is misdocumented, and the key is supposed to be "the parameter used in --set-string", not "the parameter used in values files".

@zzxwill
Copy link

zzxwill commented Sep 17, 2020

Yeah, looks like artifactOverrides is misdocumented, and the key is supposed to be "the parameter used in --set-string", not "the parameter used in values files".

Only your answer is right, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/deploy deploy/helm help wanted We would love to have this done, but don't have the bandwidth, need help from contributors july-chill kind/feature-request priority/p1 High impact feature/bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants