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

improved user-facing error messages to explain the underlying problem #8731

Conversation

technosophos
Copy link
Member

Signed-off-by: Matt Butcher matt.butcher@microsoft.com

What this PR does / why we need it:

The issue #7747 is a little bit meandering, but at root, the solution is to fix our error messages to be clearer on why a chart load failed. This replaces a terse error message with one that is more easily understood.

I added some tests, and also modified some existing tests.

Special notes for your reviewer:

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

@technosophos technosophos self-assigned this Sep 14, 2020
@helm-bot helm-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 14, 2020
@bridgetkromhout
Copy link
Member

This seems clearer, in that you are explicit about the name of the file Helm is expecting to find here. If someone has named their file with another name, that could provide a pointer as to what to fix.

@bacongobbler
Copy link
Member

bacongobbler commented Sep 14, 2020

This certainly clarifies the error message better for users of the CLI, and I appreciate the effort to improve the error message for these users.

However, at the point where c.Validate() is called, we've already loaded the chart in memory. Users who consume the SDK may be "loading" a chart from a JSON stream or constructing the chart in some manner that would not utilize a Chart.yaml. Some might even construct a chart by hand like we do in the unit tests. This error message may confuse those users as no Chart.yaml was consumed while loading their chart.

Is there a way we can instead return this error message during LoadFiles instead, before Validate is called? That way we can enforce the validation when a Chart.yaml is not present in an archive/directory.

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>
@technosophos technosophos force-pushed the fix/7747-better-error-for-missing-chart-yaml branch from a96ceea to fac05d6 Compare September 15, 2020 19:02
@technosophos
Copy link
Member Author

@bacongobbler Done.

@bacongobbler
Copy link
Member

LGTM!

@mattfarina mattfarina added this to the 3.4.0 milestone Sep 25, 2020
@technosophos technosophos merged commit 5f3e560 into helm:master Oct 19, 2020
@technosophos technosophos deleted the fix/7747-better-error-for-missing-chart-yaml branch October 19, 2020 18:16
zak905 pushed a commit to zak905/helm that referenced this pull request Jan 19, 2023
…helm#8731)

Signed-off-by: Matt Butcher <matt.butcher@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants