fix(mesheryctl-generate): Fix error not handled properly in model generate subcommand #15666
Conversation
…erate subconmad + refactoring to make code more readable Signed-off-by: lekaf974 <matthieu.evrin@gmail.com>
There was a problem hiding this comment.
Summary of Changes
Hello @lekaf974, 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 refactors the mesheryctl model generate command by introducing a structured approach for model generation from URLs and CSV files. It extracts the generation logic into dedicated generator types, improving code readability and maintainability. Crucially, it also fixes an issue where errors during model generation were not properly propagated, ensuring that failures are now correctly reported.
Highlights
- Refactoring
mesheryctl model generate: Themesheryctl model generatecommand has been refactored to improve code organization and readability by abstracting model generation logic into distinct types. - Improved Error Handling: Errors encountered during model generation, previously suppressed, are now properly returned and propagated, ensuring accurate reporting of failures.
- Introduction of Generator Types: New
ModelGeneratorinterface,UrlModelGenerator, andCsvModelGeneratorstructs have been introduced to encapsulate URL-based and CSV-based model generation logic, respectively.
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request refactors the model generation logic by introducing a ModelGenerator interface and specific implementations for URL and CSV-based generation. This significantly improves the readability and maintainability of the generate subcommand. The core issue of improper error handling, where errors were logged but suppressed, has been fixed by correctly propagating them. I've added one suggestion to further reduce code duplication in the new CsvModelGenerator.
|
|
…ve readability and maintaibility Signed-off-by: lekaf974 <matthieu.evrin@gmail.com>
| @@ -48,58 +65,41 @@ mesheryctl model generate --f [URL] -t [path-to-template.json] -r | |||
|
|
|||
| register, _ := cmd.Flags().GetBool("register") | |||
|
|
|||
| // Path is a url | |||
| if isUrl { | |||
| template, _ := cmd.Flags().GetString("template") | |||
There was a problem hiding this comment.
show error if the temple provided is invalid or empty
There was a problem hiding this comment.
this is exactly what we are doing
if template == "" {
return ErrTemplateFileNotPresent()
}|
|
||
| fileData, err := os.ReadFile(u.TemplateFile) | ||
| if err != nil { | ||
| return utils.ErrFileRead(err) |
There was a problem hiding this comment.
so this fixes the silent returned error issue right?
| } | ||
|
|
||
| err = registerModel(fileData, nil, nil, "", "url", u.Url, !u.SkipRegister) | ||
| if err != nil { |
There was a problem hiding this comment.
adding error log after nil would help
There was a problem hiding this comment.
this is what we do
return err| for _, f := range filePaths { | ||
| *f.data, err = os.ReadFile(f.path) | ||
| if err != nil { | ||
| return utils.ErrFileRead(err) |
There was a problem hiding this comment.
here as well, logging the error after this
There was a problem hiding this comment.
Return the error will automatically log it in RunE
Notes for Reviewers
Signed commits
New behavior
~$ ./mesheryctl model generate -f tests/e2e/002-model/fixtures/model-import/valid-model Error: inside the directory tests/e2e/002-model/fixtures/model-import/valid-model either the model csv or component csv is missing or they are not of write format