Fixes non-escaped comma bug on Helm command arguments

[simster7]

Fixes #1660, pretty self explanatory

[codecov]

Codecov Report

Merging #1720 into master will increase coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1720      +/-   ##
==========================================
+ Coverage   34.17%   34.26%   +0.09%     
==========================================
  Files          77       77              
  Lines       11433    11437       +4     
==========================================
+ Hits         3907     3919      +12     
+ Misses       6985     6973      -12     
- Partials      541      545       +4

Impacted Files	Coverage Δ
util/helm/helm.go	63.04% <100%> (+1.1%)	⬆️
server/server.go	46.74% <0%> (-0.24%)	⬇️
controller/appcontroller.go	39.72% <0%> (+1.39%)	⬆️

---

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 3f14f75...4d004ba. Read the comment docs.

[codecov]

Codecov Report

Merging #1720 into master will increase coverage by 0.09%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1720      +/-   ##
==========================================
+ Coverage   34.17%   34.26%   +0.09%     
==========================================
  Files          77       77              
  Lines       11433    11437       +4     
==========================================
+ Hits         3907     3919      +12     
+ Misses       6985     6973      -12     
- Partials      541      545       +4

Impacted Files	Coverage Δ
util/helm/helm.go	63.04% <100%> (+1.1%)	⬆️
server/server.go	46.74% <0%> (-0.24%)	⬇️
controller/appcontroller.go	39.72% <0%> (+1.39%)	⬆️

---

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 3f14f75...cd34ce9. Read the comment docs.

[alexec]

Hi,

Looks good - I think we need at least one test please.

Thank you,

Alex

[simster7]

@alexec Done. Seems like the linter ran out of memory on the workflow check, but a local run of golangci-lint run --fix --verbose confirms there are no problems.

[jannfis]

Hm, I was wondering whether this will break existing parameters that have been given with an escaped comma already, i.e. if somebody had manually worked around this issue "the Helm way" already.
Would that change actually escape the backslash? For example, if there'd be a parameter such as 'foo\,bar', would it turn it into 'foo\\,bar'?

Also, what will happen once Helm changes this behaviour (as they have indicated in their bug reports, that they will change it one day)?

[simster7]

@jannfis, fair point. I will add a check to ensure that commas don't get double-escaped.

[simster7]

@jannfis Fixed the double-escaping issue. The failed test is seems unrelated.

As for your other point, it seems like Helm won't break backwards compatibility without a major version bump (helm/helm#1556 (comment)). Until that decision is announced, there is no way to know how to move forward.

Unless the issue here is whether this should be fixed from our end at all. If we provide a superset to helm and we provide this fix for them, then we could run into the scenario where users using ArgoCD with what they think are valid helm configs are not actually valid. By forcing users to escape their own commas, and it is made clear that the issue comes from helm, there is no such divergence.