feat(helm): helm values can be either a string or a map (#2936)

[alexef]

fixes: #2936

Reviving #9079. I tried initially to rebase that one on master and failed miserabily.

The new approach uses a rawExtension directly, without introducing an extra StringOrObject type. Had issues with codegen locally, let me know if this is what you want @crenshaw-dev .

later edit

using a single field (StringOrObject) hit a roadblock: structural kubernetes CRD forbid it. in this latest iteration, I'm introducing a new field called valuesObject.

Signed-off-by: Alex Eftimie alex.eftimie@getyourguide.com

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• Optional. My organization is added to USERS.md.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).

[codecov]

Codecov Report

Patch coverage: 58.00% and no project coverage change.

Comparison is base (793a818) 49.60% compared to head (244da41) 49.61%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #11538   +/-   ##
=======================================
  Coverage   49.60%   49.61%           
=======================================
  Files         257      258    +1     
  Lines       44151    44192   +41     
=======================================
+ Hits        21903    21925   +22     
- Misses      20093    20106   +13     
- Partials     2155     2161    +6     

Impacted Files	Coverage Δ
cmd/util/app.go	56.22% <0.00%> (-0.21%)	⬇️
pkg/apis/application/v1alpha1/values.go	55.26% <55.26%> (ø)
cmd/argocd/commands/app.go	21.85% <100.00%> (+0.03%)	⬆️
pkg/apis/application/v1alpha1/types.go	59.11% <100.00%> (ø)
reposerver/repository/repository.go	59.26% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[crenshaw-dev]

@alexef looks like the integration test failure might be a flake.

[alexef]

It’s not flake, I can reproduce the issue with unset - so I’m doing something wrong, will need a bit more time to understand what is going on.

[alexef]

I need more time with this, converted to draft. while unit tests pass, the failing e2e point to some missing transformation or wrong assumption.

[alexef]

@crenshaw-dev I've merged master into this branch. Should be ready for a first round of review testing :)

[ishitasequeira]

Are we planning to replace values field with valueObjects completely? What would happen if user specifies both Values field and ValueObject field. Do we combine this values?

[alexef]

@ishitasequeira yes, the idea is to replace values with valuesObject completely. if an user specifies both, only valuesObject is persisted and respected.

[ishitasequeira]

Can we combine this 2 if statements into 1 as well?

[ishitasequeira]

Also wondering if we need to add some documentation around this feature?

[cod-r]

Will this make it to 2.7?

[crenshaw-dev]

@cod-r I'm afraid not. @alexef do you have time to work on this for 2.8?

[alexef]

@crenshaw-dev, yes, I should find time to work on this in 2.8, but if someone else wants to pick it up. the conflicts with master are the hardest to fix every time this drifts away

[SmaineTF1]

Looking forward seeing this released :) . Currently I have to duplicate a bunch of values from a cluster to another.

[blakepettersson]

@alexef if you don't have time to look at this I can take this on

[alexef]

@blakepettersson please go ahead! I'm granting you access to my fork, or if you want you can just fork the branch

[asaf400]

@alexef
I have an application that has used spec.source.helm.values: | as a string,
Upon redeploying argocd using the latest rc image which includes the merged PR v2.8.0-rc1 (quay.io/argoproj/argocd:v2.8.0-rc1)
I have attempted to modify the application in-place using argocd ui, but changing values: | to valuesObject I have encountered an unexpected error:

Unable to save changes: application spec for openshift-console is invalid: InvalidSpecError: Unable to generate manifests in : rpc error: code = Unknown desc = Manifest generation error (cached): 
`helm template . --name-template openshift-console --namespace openshift-console --kube-version 1.23 --api-versions all.of.k8s/v0/apis.here --include-crds` failed exit status 1: 
Error: execution error at (openshift-console/templates/deployment.yaml:1:70): .Values.bridgeEnvironmentVariables.BRIDGE_BASE_ADDRESS is required. Use --debug flag to render out invalid YAML

The chart I've used is my own helm for okd|openshift console deployment
Within the chart's app deployment template I use this templating rule to enforce a required value:

{{- $name := .Values.bridgeEnvironmentVariables.BRIDGE_BASE_ADDRESS | required ".Values.bridgeEnvironmentVariables.BRIDGE_BASE_ADDRESS is required." -}}

https://github.com/asaf400/openshift-console-chart/blob/main/charts/console/templates/deployment.yaml#L1

It works fine with values as a a string, but failed on that line when using valuesObject
I tried digging into this PR to see what would cause this, but I can't see where potentially the issue would be..

For completes, this is a sample of the values (already into the .values key):

        bridgeEnvironmentVariables:
          BRIDGE_BASE_ADDRESS: https://console.somedomain.here.dom
          BRIDGE_USER_AUTH_OIDC_CLIENT_ID: deface33-defa-cede-face-defacedeface
          BRIDGE_USER_AUTH_OIDC_ISSUER_URL: https://oauth.id.authproviderdomain.com/
        extraEnv:
          - name: BRIDGE_INACTIVITY_TIMEOUT
            value: "3600"
          - name: BRIDGE_CUSTOM_PRODUCT_NAME
            value: "Tab Title here"
        ingress:
          hosts:
            - host: console.somedomain.here.dom # value will be replaced by a patch
              paths:
                - path: /*
                  pathType: ImplementationSpecific
          sslRedirect: true

edits: typo

[crenshaw-dev]

@asaf400 thanks for reporting the problem! Would you mind opening a new issue to track it?

[asaf400]

@crenshaw-dev Not at all, I'll just copy-pasta my message from here, It's descriptive enough

edit: mention
edit 2: Opened: #11538 (comment)