-
Notifications
You must be signed in to change notification settings - Fork 162
files: improve error for uncompressed Helm .tar archives #814
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
base: master
Are you sure you want to change the base?
files: improve error for uncompressed Helm .tar archives #814
Conversation
|
Yay, your first pull request! 👍 A contributor will be by to give feedback soon. In the meantime, you can find updates in the #github-notifications channel in the community Slack. |
Summary of ChangesHello @bashSunny101, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the user experience when working with Helm charts by introducing more specific and actionable error messages. Previously, attempting to load an uncompressed ".tar" file as a Helm chart would result in a cryptic "gzip: invalid header" error. This change now explicitly identifies such cases, providing a clear explanation that Helm requires gzipped archives (.tgz or .tar.gz) and offers practical solutions, thereby reducing user confusion and improving debugging efficiency. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Signed-off-by: Sunny <[email protected]>
ab18dd8 to
5ecd41c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a helpful error message for when a user provides an uncompressed .tar archive for a Helm chart, which is a great improvement for user experience. The implementation is solid, and the new unit test effectively covers the new error path. I have a couple of suggestions to make the error detection more robust and to improve the quality of the test code.
…dling Signed-off-by: Sunny <[email protected]>
78138b9 to
d276481
Compare
|
@Darshan174 hi, |
lekaf974
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small comments added, LGTM otherwise.
- Rename ErrUncompressedTarProvided to ErrUncompressedTar - Add dedicated error code ErrUncompressedTarCode (meshkit-11305) - Simplify error function to use inline values (follow codebase pattern) - Replace generic fmt.Errorf with meshkit error ErrInvalidHelmChart - Update test to check for new error code Signed-off-by: Sunny <[email protected]>
|
I have made requested changes to this PR, kindly review it. |
| if errors.GetCode(err) != files.ErrUncompressedTarCode { | ||
| t.Fatalf("expected error code %s, got %s (err: %v)", files.ErrUncompressedTarCode, errors.GetCode(err), err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if errors.GetCode(err) != files.ErrUncompressedTarCode { | |
| t.Fatalf("expected error code %s, got %s (err: %v)", files.ErrUncompressedTarCode, errors.GetCode(err), err) | |
| assert.NotNil(t, err) // to replace previous if | |
| assert.Equal(t, files.ErrUncompressedTarCode, errors.GetCode(err)) |
| ErrUncompressedTarCode, | ||
| errors.Critical, | ||
| []string{fmt.Sprintf("The file '%s' appears to be an uncompressed TAR archive.", fileName)}, | ||
| []string{fmt.Sprintf("Helm expects chart archives to be compressed (e.g., .tgz or .tar.gz). Error details: %s", err.Error())}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| []string{fmt.Sprintf("Helm expects chart archives to be compressed (e.g., .tgz or .tar.gz). Error details: %s", err.Error())}, | |
| []string{fmt.Sprintf("Expected archives to be compressed (e.g., .tgz or .tar.gz). Error details: %s", err.Error())}, |
| errors.Critical, | ||
| []string{fmt.Sprintf("The file '%s' appears to be an uncompressed TAR archive.", fileName)}, | ||
| []string{fmt.Sprintf("Helm expects chart archives to be compressed (e.g., .tgz or .tar.gz). Error details: %s", err.Error())}, | ||
| []string{"The archive was created as an uncompressed .tar, but Helm requires compressed archives."}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| []string{"The archive was created as an uncompressed .tar, but Helm requires compressed archives."}, | |
| []string{"The archive was created as an uncompressed .tar, but archives required to be compressed."}, |
| []string{"The archive was created as an uncompressed .tar, but Helm requires compressed archives."}, | ||
| []string{ | ||
| "Compress the .tar file to .tgz or .tar.gz and try again.", | ||
| "Create the Helm chart archive using 'helm package' or gzip the archive before uploading.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| "Create the Helm chart archive using 'helm package' or gzip the archive before uploading.", | |
| "If it is a Helm chart archive, create using 'helm package' or gzip the archive before uploading.", |
lekaf974
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another review after we will be good to merged
Good job
PR description
Summary
.tar(not a gzipped chart) to Helm chart parsing and returns a clear, actionable error telling them to use.tgz/.tar.gz(orhelm package).What changed
ErrUncompressedTarProvidedto surface a helpful message and remedies when Helm's gzip reader fails.ParseFileAsHelmChart(files/identification.go) to detect the"gzip: invalid header"failure for.tarand return the new error.TestUncompressedHelmTarError(files/tests/sanitization_test.go) that builds an in-memory uncompressed tar containingChart.yamland asserts the new error path.Files touched
files/error.go— new error constructor for uncompressed.tar.files/identification.go— improved detection and error handling aroundloader.LoadArchive.files/tests/sanitization_test.go— added unit test.Why
.tgz). When an uncompressed.taris supplied the gzip reader fails with an opaque "gzip: invalid header". This change makes the failure actionable for users.Tests
.tarcase.go test ./...locally; tests pass.Related
How to validate locally
go test ./...