Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ci: Adds sync tool for hack and charts #819

Closed
wants to merge 25 commits into from
Closed

Conversation

faiq
Copy link
Contributor

@faiq faiq commented Jul 23, 2024

What problem does this PR solve?:

  • Splits the templates helm templates to read from files instead of embedding in the configmap. This makes it easier to read and modify the templates feat: Extract CAAPH values templates to files #809
  • Syncs hack/ and charts/ helm values to avoid drift. An explanation of the tool can be found in the README file in hack/tools/sync-helm-values

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:

@faiq faiq force-pushed the faiq/helm-values-include branch 2 times, most recently from a4c7a9a to fa0a3d6 Compare July 23, 2024 03:25
@faiq faiq requested a review from jimmidyson July 23, 2024 03:43
@faiq faiq force-pushed the faiq/helm-values-include branch from d46ebee to 4e62ab8 Compare July 23, 2024 18:39
@faiq faiq changed the base branch from jimmi/helm-values-include to main July 23, 2024 18:44
@faiq faiq force-pushed the faiq/helm-values-include branch from 4e62ab8 to 4f64309 Compare July 23, 2024 19:11
@faiq faiq enabled auto-merge (squash) July 23, 2024 20:00
Copy link
Contributor

@supershal supershal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally by modifying files in /hack/addons/kustomize and syncing the values.

@faiq faiq requested a review from dlipovetsky July 25, 2024 18:54
@faiq faiq force-pushed the faiq/helm-values-include branch from 28da0c0 to 4c52882 Compare July 26, 2024 18:57
@dlipovetsky
Copy link
Contributor

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
Copy link
Contributor Author

faiq commented Jul 30, 2024

@dlipovetsky can you run the make target?

make sync-helm-values

@jimmidyson jimmidyson changed the title ci: addds sync tool for hack and charts ci: Adds sync tool for hack and charts Aug 13, 2024
@faiq faiq force-pushed the faiq/helm-values-include branch 2 times, most recently from 9b73715 to d1571b8 Compare August 14, 2024 10:03
.pre-commit-config.yaml Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@jimmidyson
Copy link
Member

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
Copy link
Contributor Author

faiq commented Aug 14, 2024

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.

@faiq faiq requested a review from jimmidyson August 14, 2024 18:40
Copy link
Member

@jimmidyson jimmidyson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a few issues with the CA UUID annotation usage - please can you double check the changes related there.

@dlipovetsky
Copy link
Contributor

dlipovetsky commented Aug 29, 2024

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
Copy link
Member

jimmidyson commented Aug 30, 2024

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
Copy link
Member

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
Copy link
Member

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.

@faiq faiq closed this Aug 30, 2024
auto-merge was automatically disabled August 30, 2024 18:40

Pull request was closed

jimmidyson added a commit that referenced this pull request Sep 7, 2024
This simplifies the Helm templating directives by not requiring
inception-style escaping of templating braces, e.g. `{{ "{{" }}``
which are very hard to read and can introduce bugs.

This PR also removes the duplicate helm values files currently being
used
to generate the CRS configmaps, and instead references the helm values
that are in the charts directory, which ends up with a reduction in LOC
in
the project to maintain.

I feel this is a simpler way to achieve the same goals as #819 but
without
duplicating files and keeping all chart source files in the charts
directory.

Blocked by #895.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants