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

Fix Helm values parsing #474

Merged
merged 1 commit into from
Dec 1, 2020

Conversation

jnummelin
Copy link
Member

@jnummelin jnummelin commented Nov 30, 2020

Signed-off-by: Jussi Nummelin jnummelin@mirantis.com

Issue
Fixes #447

What this PR Includes
Intorduces a cleanup function to make all nested maps being converted into map[string]interface{} types. By default golang parses the nested maps into map[interface{}]interface{} types which makes Helm to actually ignore those keys when merging the chart default values into given values.

in the Helm code which merges the defaults in, there’s this:

if dest, ok := value.(map[string]interface{}); ok {

see: https://github.com/helm/helm/blob/6aefbdcf38e14998d0bcb24030061822b1e8888d/pkg/chartutil/coalesce.go#L156-L157

The cast is not successfull with map[interface{}]interface{} type and thus no merging actually happened

Adds also a verification into the related smoke case that we really see the given values being propagated into place.

@jnummelin jnummelin added bug Something isn't working area/controlplane labels Nov 30, 2020
@jnummelin jnummelin added this to the 0.8.0 milestone Nov 30, 2020
@jnummelin jnummelin requested a review from a team as a code owner November 30, 2020 22:37
@jnummelin jnummelin requested review from kke and jasmingacic and removed request for a team November 30, 2020 22:37
@jnummelin jnummelin force-pushed the fix-helm-values-parsing branch from f31eb0a to f819a5b Compare November 30, 2020 22:44
@jnummelin jnummelin requested review from trawler and removed request for kke November 30, 2020 22:44
jasmingacic
jasmingacic previously approved these changes Dec 1, 2020
ncopa
ncopa previously requested changes Dec 1, 2020
Copy link
Collaborator

@ncopa ncopa left a comment

Choose a reason for hiding this comment

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

Congrats with a good catch!

Signed-off-by: Jussi Nummelin <jnummelin@mirantis.com>
@jnummelin
Copy link
Member Author

@ncopa @jasmingacic Had to force push and thus dismiss the reviews, could you PTAL again, thanks.

Copy link
Contributor

@jasmingacic jasmingacic left a comment

Choose a reason for hiding this comment

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

I just tested this setup:

charts:
    - name: akri-dev
      chartname: akri/akri-dev
      version: "0.0.42"
      namespace: akri
      values: |
        debugEcho:
          enabled: true
          shared: false
        agent:
          allowDebugEcho: true
          host:
            dockerShimSock: /my/new/docker/shim/foo.sock

Which resulted with:

# sudo crictl --runtime-endpoint unix:///var/lib/k0s/run/containerd.sock ps
CONTAINER           IMAGE               CREATED             STATE               NAME                         ATTEMPT             POD ID
2e50e5244dcc4       c0bd9c7f70f66       2 seconds ago       Running             akri-controller              2                   fe89ca98b8eba
ac6573b365fd9       37c2efaddb793       2 seconds ago       Running             akri-agent                   2   

And this:

# sudo crictl --runtime-endpoint unix:///var/lib/k0s/run/containerd.sock inspect ac6573b365fd9 | grep -i hostpath
         ...
        "hostPath": "/my/new/docker/shim/foo.sock",

Looking good

@jnummelin jnummelin requested a review from ncopa December 1, 2020 09:04
@jnummelin jnummelin dismissed ncopa’s stale review December 1, 2020 09:14

requested changes force pushed

@jnummelin jnummelin merged commit 8122c5c into k0sproject:main Dec 1, 2020
@jnummelin jnummelin deleted the fix-helm-values-parsing branch December 1, 2020 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/controlplane bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Helm extension not applying values
3 participants