-
Notifications
You must be signed in to change notification settings - Fork 166
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
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 <77166237+bashSunny101@users.noreply.github.com>
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 <77166237+bashSunny101@users.noreply.github.com>
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 <77166237+bashSunny101@users.noreply.github.com>
|
I have made requested changes to this PR, kindly review it. |
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
|
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
|
@bashSunny101 do you have time to do last changes so it can be merged ? |
Co-authored-by: Matthieu Evrin <6630779+lekaf974@users.noreply.github.com> Signed-off-by: Sunny Pal <77166237+bashSunny101@users.noreply.github.com>
Co-authored-by: Matthieu Evrin <6630779+lekaf974@users.noreply.github.com> Signed-off-by: Sunny Pal <77166237+bashSunny101@users.noreply.github.com>
Co-authored-by: Matthieu Evrin <6630779+lekaf974@users.noreply.github.com> Signed-off-by: Sunny Pal <77166237+bashSunny101@users.noreply.github.com>
Co-authored-by: Matthieu Evrin <6630779+lekaf974@users.noreply.github.com> Signed-off-by: Sunny Pal <77166237+bashSunny101@users.noreply.github.com>
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.
Pull request overview
This PR improves error handling for Helm chart parsing by detecting when users provide an uncompressed .tar file (instead of a gzipped .tgz or .tar.gz) and returning a clear, actionable error message explaining how to fix the issue.
- Introduces a new error type
ErrUncompressedTarwith helpful remediation steps - Updates
ParseFileAsHelmChartto detect gzip header failures and provide specific guidance - Adds comprehensive test coverage for the uncompressed tar scenario
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
files/error.go |
Adds new error code and constructor ErrUncompressedTar with actionable remediation guidance |
files/identification.go |
Enhances error detection in ParseFileAsHelmChart to identify uncompressed tar files and return the new specific error |
files/tests/sanitization_test.go |
Adds unit test TestUncompressedHelmTarError that validates the new error path with an in-memory uncompressed tar |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Sunny Pal <77166237+bashSunny101@users.noreply.github.com>
- Add github.com/stretchr/testify/assert import - Replace if err == nil check with assert.NotNil - Clean up duplicate error checking code Signed-off-by: Sunny <77166237+bashSunny101@users.noreply.github.com>
|
@lekaf974 I've addressed the requested changes:
All changes have been committed with sign-off and pushed. Please let me know if there's anything else that needs to be updated. Thanks for the review! |
|
@bashSunny101 thanks and good Job |
|
Thanks for your contribution to Meshery! 🎉
|
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 ./...