bugfix: unexpected behavior when using "dependencies.condition" #11396
Conversation
d108c78 to
623f4c6
Compare
pkg/chartutil/dependencies.go
Outdated
| for _, r := range c.Metadata.Dependencies { | ||
| if !r.Enabled { | ||
| // remove disabled chart | ||
| rm[r.Name] = struct{}{} | ||
| rm[r.Name] = append(rm[r.Name], r.Version) |
There was a problem hiding this comment.
Hi @wujunwei, would you please take into consideration using chart alias from the dependencies as well? When I have a helm chart with two dependencies for chart X, version 0.0.1, but with different aliases, it will still break conditionals.
Chart.yaml
apiVersion: v2
name: my-app-configuration
version: 0.0.1
dependencies:
- name: my-service-chart
alias: firstService
version: 0.3.0
repository: "@stable"
condition: firstService.enabled
- name: my-service-chart
alias: secondService
version: 0.3.0
repository: "@stable"
condition: secondService.enabled
There was a problem hiding this comment.
In fact, I have considered this situation.
As the code below shows, when deal with package's name ,it have considered the alias name.
helm/pkg/chartutil/dependencies.go
Lines 107 to 115 in 623f4c6
There was a problem hiding this comment.
@kszpakowski I added some test cases To prove my point. PTAL :)
There was a problem hiding this comment.
Thanks, your change looks good to me. Unfortunately it looks like I'm facing another issue and it still does not work on your version. Basically, I have a chart with two dependencies to another chart, which then have its own conditional dependencies disabled by default in its values.yaml. When I render it, all resources are created ignoring conditions. In another words, condition in dependency of my chart dependencies is being ignored. I'll create example repo and open new issue.
There was a problem hiding this comment.
I've just found the solution in the docs: "Tags and conditions values must be set in the top parent's values." :)
There was a problem hiding this comment.
top parent's values
Could you send an example
There was a problem hiding this comment.
It looks like I was too optimistic. Today I had more time to investigate the issue and it looks like there is some bug after all. I have charts structure like this:
ChartA
ChartB
dependencies:
- name: ChartA
alias: chartA
condition: chartA.enabled
ChartC
dependencies:
- name: ChartB
alias: firstApp - name: ChartB
alias: secondApp
Top most chart (ChartC) has two dependencies to the same lib chart (ChartB), same version, different aliases.
ChartB has single dependency (which is conditional) to ChartA with an alias.
I do not know go good enough to propose a fix, but it looks like, the first time getAliasDependency is called and it modifies dep name to the alias (md.Name = dep.Alias). When it is called again, dep name is already set to an alias, and it doesn't match c.Name (Line 98) - the metadata name is not being changed to an alias. Then there is a mismatch between rm content and it cannot get the right value from rm in line 173, so the disabled by condition dependency is being kept.
My temporary solution was to remove alias from dependency of chart B and now it works fine.
There was a problem hiding this comment.
the first time getAliasDependency is called and it modifies dep name to the alias (md.Name = dep.Alias). When it is called again, dep name is already set to an alias, and it doesn't match c.Name (Line 98)
why does it not match c.Name? when called again, it's another dep.
the metadata name is not being changed to an alias. Then there is a mismatch between rm content and it cannot get the right value from rm in line 173
n.Metadata.Name in line 173 comes from c.Dependencies(0 which reset by line 147, it has already changed to alias from the code point of view.
Have you test it? could you provide a test case that can reproduct it ? thanks a lot !
There was a problem hiding this comment.
I made this sample to reproduce the error: https://github.com/kszpakowski/helm-alias-condition-error
There was a problem hiding this comment.
I made this sample to reproduce the error: https://github.com/kszpakowski/helm-alias-condition-error
Thanks for this demo, It can be produced at the latest main branch
phenomenon : when I run helm template chartC output contained sa.yaml which is disabled at chartC's values.yaml .
> helm template ./helm-alias-condition-error/chartC/
---
# Source: chartC/charts/firstApp/templates/ns.yaml
apiVersion: v1
kind: Namespace
metadata:
name: test
---
# Source: chartC/charts/secondApp/templates/ns.yaml
apiVersion: v1
kind: Namespace
metadata:
name: test
---
# Source: chartC/charts/secondApp/charts/chartA/templates/sa.yaml
apiVersion: v1
kind: ServiceAccount
metadata:
name: test
After Looking into the code ,I found , It's cause by code below. it tried to copy a chart, but the medata.dependencies is a array of chart point,it will copy reference not truly struct. as a result they will share same address, when change the name of firstApp.chartA to firstApp.chartAalias secondApp.chartA will be set to secondApp.chartAalias at the same time.
helm/pkg/chartutil/dependencies.go
Lines 105 to 112 in 4886066
It's a new bug(although the phenomenon is same as this issue), you can submit a new issue to trace it .
623f4c6 to
a382ecb
Compare
a382ecb to
02df0c2
Compare
02df0c2 to
d73100c
Compare
d73100c to
210a651
Compare
210a651 to
b7596b7
Compare
b7596b7 to
18e3050
Compare
a265a65 to
30f422f
Compare
30f422f to
62fb87e
Compare
62fb87e to
219857c
Compare
|
@mattfarina @joejulian could anyone take a look ? Another new user has encountered the same bug again! |
Signed-off-by: wujunwei <wjw3323@live.com> Signed-off-by: adam.wu <adamwu@shein.com> # Conflicts: # pkg/chart/v2/util/dependencies_test.go
Signed-off-by: wujunwei <wjw3323@live.com>
219857c to
9514321
Compare
Signed-off-by: wujunwei <wjw3323@live.com> Signed-off-by: adam.wu <adamwu@shein.com>
e7efaa4 to
7e5f575
Compare
Signed-off-by: wujunwei wjw3323@live.com
What this PR does / why we need it:
fixes #11338
Special notes for your reviewer:
If applicable: