Fix value precedence#10110
Merged
Merged
Conversation
Fix value precedence when importing values from child. Closes helm#9940 Signed-off-by: Aram Zegerius <[email protected]>
helm-bot
added
the
size/XS
Member
|
Can you please provide a test case to confirm #9940 has been fixed? |
Contributor
Author
|
@bacongobbler sure, I'll do that tomorrow together with my colleague @ankitabhopatkar13. This was tested against this repository https://github.com/ankitabhopatkar13/helm-values-issue. AFAIK no tests broke, I am not sure how often this code block is hit. |
Closes helm#9940 Signed-off-by: Aram Zegerius <[email protected]> Signed-off-by: Ankita Bhopatkar <[email protected]>
helm-bot
added
size/XL
Contributor
Author
|
@bacongobbler - @ankitabhopatkar13 and I finalized the test. Output before and after the fix below: Before fix After fix |
Member
|
Tested manually with chart from linked issue: https://github.com/ankitabhopatkar13/helm-values-issue Before fix:
$ helm upgrade --install umbrella umbrella --debug --dry-run
MANIFEST:
---
# Source: umbrella/charts/app1/templates/service.yaml
apiVersion: v1
kind: Service
metadata:
name: umbrella-app1
labels:
helm.sh/chart: app1-0.1.0
app.kubernetes.io/name: app1
app.kubernetes.io/instance: umbrella
app.kubernetes.io/version: "1.16.0"
app.kubernetes.io/managed-by: Helm
spec:
type: ClusterIP
ports:
- port: 1234
targetPort: http
protocol: TCP
name: http
selector:
app.kubernetes.io/name: app1
app.kubernetes.io/instance: umbrella
---
# Source: umbrella/charts/app2/templates/service.yaml
apiVersion: v1
kind: Service
metadata:
name: umbrella-app2
labels:
helm.sh/chart: app2-0.1.0
app.kubernetes.io/name: app2
app.kubernetes.io/instance: umbrella
app.kubernetes.io/version: "1.16.0"
app.kubernetes.io/managed-by: Helm
spec:
type: ClusterIP
ports:
- port: 8080
targetPort: http
protocol: TCP
name: http
selector:
app.kubernetes.io/name: app2
app.kubernetes.io/instance: umbrella
$ helm upgrade --install umbrella umbrella --debug --dry-run --values umbrella/values.yaml
MANIFEST:
---
# Source: umbrella/charts/app1/templates/service.yaml
apiVersion: v1
kind: Service
metadata:
name: umbrella-app1
labels:
helm.sh/chart: app1-0.1.0
app.kubernetes.io/name: app1
app.kubernetes.io/instance: umbrella
app.kubernetes.io/version: "1.16.0"
app.kubernetes.io/managed-by: Helm
spec:
type: ClusterIP
ports:
- port: 9090
targetPort: http
protocol: TCP
name: http
selector:
app.kubernetes.io/name: app1
app.kubernetes.io/instance: umbrella
---
# Source: umbrella/charts/app2/templates/service.yaml
apiVersion: v1
kind: Service
metadata:
name: umbrella-app2
labels:
helm.sh/chart: app2-0.1.0
app.kubernetes.io/name: app2
app.kubernetes.io/instance: umbrella
app.kubernetes.io/version: "1.16.0"
app.kubernetes.io/managed-by: Helm
spec:
type: ClusterIP
ports:
- port: 9090
targetPort: http
protocol: TCP
name: http
selector:
app.kubernetes.io/name: app2
app.kubernetes.io/instance: umbrellaAfter fix:
$ helm upgrade --install umbrella umbrella --debug --dry-run
MANIFEST:
---
# Source: umbrella/charts/app1/templates/service.yaml
apiVersion: v1
kind: Service
metadata:
name: umbrella-app1
labels:
helm.sh/chart: app1-0.1.0
app.kubernetes.io/name: app1
app.kubernetes.io/instance: umbrella
app.kubernetes.io/version: "1.16.0"
app.kubernetes.io/managed-by: Helm
spec:
type: ClusterIP
ports:
- port: 1234
targetPort: http
protocol: TCP
name: http
selector:
app.kubernetes.io/name: app1
app.kubernetes.io/instance: umbrella
---
# Source: umbrella/charts/app2/templates/service.yaml
apiVersion: v1
kind: Service
metadata:
name: umbrella-app2
labels:
helm.sh/chart: app2-0.1.0
app.kubernetes.io/name: app2
app.kubernetes.io/instance: umbrella
app.kubernetes.io/version: "1.16.0"
app.kubernetes.io/managed-by: Helm
spec:
type: ClusterIP
ports:
- port: 8080
targetPort: http
protocol: TCP
name: http
selector:
app.kubernetes.io/name: app2
app.kubernetes.io/instance: umbrella
$ helm upgrade --install umbrella umbrella --debug --dry-run --values umbrella/values.yaml
MANIFEST:
---
# Source: umbrella/charts/app1/templates/service.yaml
apiVersion: v1
kind: Service
metadata:
name: umbrella-app1
labels:
helm.sh/chart: app1-0.1.0
app.kubernetes.io/name: app1
app.kubernetes.io/instance: umbrella
app.kubernetes.io/version: "1.16.0"
app.kubernetes.io/managed-by: Helm
spec:
type: ClusterIP
ports:
- port: 1234
targetPort: http
protocol: TCP
name: http
selector:
app.kubernetes.io/name: app1
app.kubernetes.io/instance: umbrella
---
# Source: umbrella/charts/app2/templates/service.yaml
apiVersion: v1
kind: Service
metadata:
name: umbrella-app2
labels:
helm.sh/chart: app2-0.1.0
app.kubernetes.io/name: app2
app.kubernetes.io/instance: umbrella
app.kubernetes.io/version: "1.16.0"
app.kubernetes.io/managed-by: Helm
spec:
type: ClusterIP
ports:
- port: 8080
targetPort: http
protocol: TCP
name: http
selector:
app.kubernetes.io/name: app2
app.kubernetes.io/instance: umbrellaNotice the I think this fixes the issue and test case passed during local run. |
Contributor
Author
|
I'll simplify the test data directory. There's a bit too many adjacent things going on there. |
Signed-off-by: Matthew Fisher <[email protected]>
Problem: the warnings don't give enough details about which values are problematic, only the name of the leaf key. This is all the more annoying when you have a chart depending on other charts. ``` mainchart | +- subchart1 +- subchart2 +- subchart3 ``` Here are some warnings I get before the change: ``` coalesce.go:199: warning: destination for credentials is a table. Ignoring non-table value coalesce.go:160: warning: skipped value for resources: Not a table. coalesce.go:160: warning: skipped value for googleSheetsServiceAccount: Not a table. coalesce.go:199: warning: destination for googleSheetsServiceAccount is a table. Ignoring non-table value coalesce.go:199: warning: destination for resources is a table. Ignoring non-table value [] coalesce.go:199: warning: destination for credentials is a table. Ignoring non-table value coalesce.go:199: warning: destination for credentials is a table. Ignoring non-table value coalesce.go:160: warning: skipped value for resources: Not a table. coalesce.go:160: warning: skipped value for googleSheetsServiceAccount: Not a table. ``` with fix: ``` coalesce.go:162: warning: skipped value for subchart1.resources: Not a table. coalesce.go:162: warning: skipped value for subchart2.googleSheetsServiceAccount: Not a table. coalesce.go:211: warning: destination for subchart3.aws.credentials is a table. Ignoring non-table value () coalesce.go:211: warning: destination for mainchart.subchart3.aws.credentials is a table. Ignoring non-table value () coalesce.go:211: warning: destination for mainchart.subchart2.googleSheetsServiceAccount is a table. Ignoring non-table value () coalesce.go:211: warning: destination for mainchart.subchart1.resources is a table. Ignoring non-table value ([]) coalesce.go:162: warning: skipped value for subchart1.resources: Not a table. coalesce.go:162: warning: skipped value for subchart2.googleSheetsServiceAccount: Not a table. coalesce.go:211: warning: destination for subchart3.aws.credentials is a table. Ignoring non-table value () ``` Signed-off-by: Damien Nozay <[email protected]> add tests Signed-off-by: Damien Nozay <[email protected]>
Signed-off-by: Aram Zegerius <[email protected]> Signed-off-by: Ankita Bhopatkar <[email protected]>
helm-bot
added
size/L
Signed-off-by: Aram Zegerius <[email protected]> Signed-off-by: Ankita Bhopatkar <[email protected]>
Contributor
Author
|
Simplified test data. I think it can be merged @bacongobbler @zonggen |
Member
|
Ran the test case and with |
|
Hi @mattfarina, I see this has been marked to 3.8.1 milestone instead of the 3.8.0. Is there anything pending on this MR? We have to workaround a lot because of this bug and would appreciate it released as soon as possible. Thanks! |
mattfarina
approved these changes
Apr 12, 2022
mattfarina
added
needs-pick
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix value precedence when importing values from child, as described in issue #9940
What this PR does / why we need it:
Closes #9940
If applicable: