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

fix: helm chart generates invalid YAML when watching all namespaces #812

Merged
merged 2 commits into from
Jan 30, 2024

Conversation

adutra
Copy link
Contributor

@adutra adutra commented Jan 26, 2024

When a reconciler is configured to watch all namespaces, a ClusterRoleBinding object is generated by
AddRoleBindingsDecorator, and it later gets appended to templates/kubernetes.yml by HelmChartProcessor.

However, that file becomes syntactically invalid because FileUtils.asYaml produces a top-level YAML array when passed a List. That's not what Helm expects.

When one tries to install the generated chart, the following error is returned:

 Error: INSTALLATION FAILED: YAML parse error on
 [...]/templates/kubernetes.yml: error unmarshaling JSON:
 while decoding JSON: json: cannot unmarshal array into
 Go value of type releaseutil.SimpleHead
 YAML parse error on [...]/templates/kubernetes.yml

When a reconciler is configured to watch all namespaces,
a ClusterRoleBinding object is generated by
AddRoleBindingsDecorator, and it later gets appended
to templates/kubernetes.yml by HelmChartProcessor.

However, that file becomes syntactically invalid because
FileUtils.asYaml produces a top-level YAML array when passed
a List. That's not what Helm expects.

When one tries to install the generated chart, the following
error is returned:

   Error: INSTALLATION FAILED: YAML parse error on
   [...]/templates/kubernetes.yml: error unmarshaling JSON:
   while decoding JSON: json: cannot unmarshal array into
   Go value of type releaseutil.SimpleHead
   YAML parse error on [...]/templates/kubernetes.yml
@metacosm metacosm requested a review from csviri January 26, 2024 18:19
@adutra
Copy link
Contributor Author

adutra commented Jan 27, 2024

@csviri I wonder if the ClusterRoleBinding doc shouldn't be filtered out by HelmChartProcessor#filterOutStandardResources? It seems we already have an equivalent ClusterRoleBinding being generated in the xyz-reconciler-crd-role-binding.yaml file.

@adutra
Copy link
Contributor Author

adutra commented Jan 27, 2024

I thought some more about this and figured that it made sense to exclude the generated cluster role binding.

I added another commit that excludes it, and enhances the test to produce some custom resources and check that they all get appended to kubernetes.yml.

Copy link
Contributor

@csviri csviri left a comment

Choose a reason for hiding this comment

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

Makes sense, and LGMT

Copy link
Member

@metacosm metacosm left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

@metacosm metacosm merged commit 018ee5e into quarkiverse:main Jan 30, 2024
5 checks passed
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.

3 participants