nutanix-cloud-native / cluster-api-runtime-extensions-nutanix

https://nutanix-cloud-native.github.io/cluster-api-runtime-extensions-nutanix/
Apache License 2.0
7 stars 4 forks source link

ci: Adds sync tool for hack and charts #819

Closed faiq closed 1 week ago

faiq commented 1 month ago

What problem does this PR solve?:

As a part of the file changes to support the sync helm-values.yaml had to be changed to use chart names as the config map field.

For example local-path-provisioner-csi local-path-provisioner`

Some changes in pkg/handlers/generic/lifecycle/ were changed to use the new names.

Which issue(s) this PR fixes: Fixes #

How Has This Been Tested?:

Special notes for your reviewer:

dlipovetsky commented 1 month ago

Just checked out the branch and ran the command, but got an error.

> go run sync-values.go \
                                                                                           -kustomize-directory=../../addons/kustomize/ \
                                                                                           -helm-chart-directory=../../../charts/cluster-api-runtime-extensions-nutanix/
failed to sync err: failed to write license to file /home/dlipovetsky/nutanix/cluster-api-runtime-extensions-nutanix/charts/cluster-api-runtime-extensions-nutanix/templates/ccm/aws/manifests/helm-addon-installation.yaml write /home/dlipovetsky/nutanix/cluster-api-runtime-extensions-nutanix/charts/cluster-api-runtime-extensions-nutanix/templates/ccm/aws/manifests/helm-addon-installation.yaml: copy_file_range: is a directory
faiq commented 1 month ago

@dlipovetsky can you run the make target?

make sync-helm-values

jimmidyson commented 4 weeks ago

I think I'm missing something, but why do we need to sync the values from hack to chart dir rather than just work with them directly in chart dir? This seems like it's duplicating files.

faiq commented 4 weeks ago

I think I'm missing something, but why do we need to sync the values from hack to chart dir rather than just work with them directly in chart dir? This seems like it's duplicating files.

Right now I think this would be helpful when updating values for addons with this new directory structure. Rather than going out and modifying them in charts/addons and charts/templates we can make all the changes for values when handling an addon in one place.

I was also able to consolidate some of the files by sharing value files for some of the addons as well.

dlipovetsky commented 1 week ago

nit: Could you please rename the various $NAME-template.yaml files to $NAME.yaml.tmpl, to help signal that the files are intended to be processed by go's text/template?

jimmidyson commented 1 week ago

As I've said to you before @faiq, my big problem with this approach is editing files that are literally copied to the chart directory in a different directory, even though both files are committed. I now have duplication for no reason in the codebase.

As an example, charts/cluster-api-runtime-extensions-nutanix/templates/ccm/nutanix/manifests/helm-addon-installation.yaml is identical to charts/cluster-api-runtime-extensions-nutanix/templates/ccm/nutanix/manifests/helm-addon-installation.yaml and you have to edit the file outside the chart to be synced to the chart directory.

I would much prefer the chart files to be edited in the charts directory - this is the charts source directory and editing the source somewhere else only to copy it back to source directory feels very odd to me, like editing go source in a different directory only to copy it to somewhere else to compile it, and committing both files to git.

I think this approach is really only here because we generate ClusterResourceSet configmap manifests for the addons. With the near term hope of removing ClusterResourceSet support and solely relying on CAAPH for addon deployment I feel we will just have to undo this method and move the chart source back to the charts directory. This seems a big change for a short term problem.

I do like the change to pull the embedded go template files used for CAAPH values out to files (as I originally did in #809). This a big win as it removes the very confusing and error prone double embedded go template stuff.

jimmidyson commented 1 week ago

I wonder if we can get the same benefits by putting the values templates only in the charts dir (to make me happy!) and just reference them there from the kustomizations via relative path? Would have to use the same more lax load restricter none we use when syncing examples, but I think that would remove them need for this values syncing completely while still gaining the other benefits of keeping values in sync between helm addon and crs (because they would reference the same values file from charts dir).

jimmidyson commented 1 week ago

I pushed a PR following what I proposed above - #896. I think this achieves the same goal as this PR (ensuring no drift between values for CRS and CAAPH addon strategies, and removes inception style templating in the helm values files), but reduces LOC and does not duplicate any files.