Add extra_values param to app_chart

[koonpeng]

Not sure if this is a good way to implement it. iiuc token-vendor is using some base values.yaml (like deploy_environment). If I define values file in the app_chart it fails to render unless I copy and extend the base values.

Add extra_values param to app_chart. Unlike values which overrides the default values, extra_values append to it instead.

[drigz]

Are you saying that if you create values.yaml adjacent to BUILD and refer to it from this target then something breaks? Can you share the exact error? I'd expect that to work:

core/src/app_charts/token-vendor/BUILD.bazel

Line 12 in 270d178

)

You might need to copy-paste

core/bazel/build_rules/app_chart/values-cloud.yaml

Line 4 in 270d178

registry: "gcr.io/my-gcp-project"

for the default values? Or at least a subset like in this example: https://github.com/googlecloudrobotics/core/blob/270d1786d30af37814accd73ea27640013bb3884/src/app_charts/k8s-relay/values-cloud.yaml

[koonpeng]

Yes, like k8s-relay. This is the error if I do not copy the base values

INFO: Analyzed target //src/app_charts/token-vendor:token-vendor-cloud (698 packages loaded, 24751 targets configured).
INFO: From Action src/app_charts/token-vendor/token-vendor-cloud-0.0.1.tgz:
Error: 1 chart(s) linted, 1 chart(s) failed
==> Linting token-vendor-cloud
[ERROR] templates/: render error in "token-vendor-cloud/templates/token-vendor.yaml": template: token-vendor-cloud/templates/token-vendor.yaml:31:7: executing "token-vendor-cloud/templates/token-vendor.yaml" at <eq .Values.deploy_environment "GCP-testing">: error calling eq: incompatible types for comparison

INFO: Found 1 target...
Target //src/app_charts/token-vendor:token-vendor-cloud up-to-date:
  bazel-bin/src/app_charts/token-vendor/token-vendor-cloud-0.0.1.tgz
INFO: Elapsed time: 1.540s, Critical Path: 0.31s
INFO: 3 processes: 564 action cache hit, 1 internal, 2 linux-sandbox.
INFO: Build completed successfully, 3 total actions

bazel still thinks the build succeeded, but I don't think it actually succeeded.

[koonpeng]

Added extra_values option in app_chart to append additional values on top of the base. This allows us to keep 1 SOT while having documentation by example.

[ensonic]

TBH. I do not understand we we need 2 values files. If we need to, it should be crystal clear from the code how it would be used and why it is beneficial.

[koonpeng]

If a values file is not given, a default values file will be used. token-vendor use values in the default values file so if we want to add more values, we need to copy and extend the default. It's just a rare case that if we want to change the default, there is no longer any single SOT.

[drigz]

I guess Stefan is asking for a cleanup that migrates the other charts to use extra_values. Otherwise people will have the same problem as before: if they copy-paste the example from k8s-relay to a chart that uses values that are not in k8s-relay's values-cloud.yaml, they'll get the same error you did.

[ensonic]

A cleanup can be in a followup, but I like to have more details on the bazel rule that explains the need.

Eg. when I look at https://github.com/googlecloudrobotics/core/tree/main/src/app_charts/token-vendor
it does not yet have any values.yaml at all.

[ensonic]

What is now the usecase for specifying values? If no rule is doing this anymore and everything is using extra_values, we can keep the attribute name to be called "values" and prepend the defaults?

[koonpeng]

These targets cannot be migrated to use extra_values because they override some base values.

core/src/app_charts/base/BUILD.bazel

Line 109 in f2648d4

values = "values-cloud.yaml",

core/src/app_charts/mission-crd/BUILD.bazel

Line 7 in f2648d4

values = "values.yaml",

[drigz]

@koonpeng Please decouple the helm_chart change from the token-vendor change: Copy the full list of required values or the subset of required values to token-vendor/values.yaml, and if you still want to make the "create a new values.yaml" flow easier for future developers, create a new PR that adds extra_values.yaml (or just changes the docs to explain where to copy the base values from for full compatibility).

[koonpeng]

Made another PR that cherry picked #672 the tv changes.

[koonpeng]

@drigz Now that #672 is merged, this PR should now be a refactor will no functionality changes.

[drigz]

Thanks, seems like a safe improvement to me. Since I misunderstood @ensonic's comment before I'll defer approval to him.

[ensonic]

If this works for all charts please merge. If it causes issues for some, please file a ticket, so that we can think about the required changes.