rancher / hull

Keep your Helm charts afloat with comprehensive testing
Apache License 2.0
4 stars 5 forks source link

[Bug] error from parser when it tries to parse NOTES.txt file of chart #35

Closed vardhaman22 closed 1 year ago

vardhaman22 commented 1 year ago

Describe the bug

While rendering templates of a chart, the parser tries to parse NOTES.txt and extract kubernetes object from it. While parsing following case is handled

https://github.com/rancher/hull/blob/824596116fec7cf909442e4186d210fe9022a1e3/pkg/parser/parser.go#L29-L32

if any other error occurs then the test case fails.

To Reproduce

1. replace the content in example chart Notes.txt file (testdata/charts/example-chart/templates/NOTES.txt) with the following content ``` Welcome to Kiali! For more details on Kiali, see: https://kiali.io The Kiali Server [{{ .Chart.AppVersion }}] has been installed in namespace [{{ .Release.Namespace }}]. It will be ready soon. (Helm: Chart=[{{ .Chart.Name }}], Release=[{{ .Release.Name }}], Version=[{{ .Chart.Version }}]) ``` 2. run the test added for example chart (examples/tests/example/example_test.go) to get the following error in the output ``` * error converting YAML to JSON: yaml: line 6: could not find expected ':' ``` **Result** Test Failed. **Expected Result**

Test Should Pass

Screenshots

Additional context

aiyengar2 commented 1 year ago

I agree with the fix proposed offline, which is to filter all sources that are not YAML in the for loop for chart.RenderTemplate here:

https://github.com/rancher/hull/blob/824596116fec7cf909442e4186d210fe9022a1e3/pkg/chart/chart.go#L83-L99

Another possible solution is to directly fix the parser package to create an empty objectset on receiving a non-YAML manifest, but IMO parser should assume the provided manifest is expected to be Kubernetes YAML.

I also think part of this fix should be to remove the specific error check referenced above, with a comment that states that Parse is only expected to be used for a valid Kubernetes YAML manifest:

https://github.com/rancher/hull/blob/824596116fec7cf909442e4186d210fe9022a1e3/pkg/parser/parser.go#L29-L32

Then on running this command: https://github.com/rancher/hull/blob/7933f196924bb1c0be7db5f494e330b1fa5bd117/pkg/chart/chart.go#L86

We should emit a more human-readable error identifying which source file actually caused this error to be emitted, so the user knows with .yaml file isn't actually producing real YAML.