Conversation
Summary of ChangesHello, 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 robustness of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly addresses the issue of ensuring a non-zero exit code when rm or rmi commands encounter errors during execution. The approach of counting errors and returning an error at the end is sound. My feedback includes a couple of suggestions to improve the clarity of the final error messages and to use more idiomatic Go for error formatting. Overall, this is a good fix.
| errMsg := fmt.Sprintf("failed to remove %d containers", errCount) | ||
| return errors.New(errMsg) |
There was a problem hiding this comment.
The error message failed to remove might not be fully accurate in all cases, as errCount is also incremented for inspection failures or if a container is not a Toolbx container. A more generic message would be more appropriate. Also, you can use fmt.Errorf to create a formatted error directly, which is more idiomatic in Go.
| errMsg := fmt.Sprintf("failed to remove %d containers", errCount) | |
| return errors.New(errMsg) | |
| return fmt.Errorf("encountered errors with %d containers", errCount) |
There was a problem hiding this comment.
It is true that we can encounter errors that may prevent us from even calling podman to remove them
But my reasoning for still sticking with the failed to remove error was:
Any error encountered during the removal process prevents the container or image from being removed. So, in essence, isn’t it a failure of the removal process as a whole?
| errMsg := fmt.Sprintf("failed to remove %d images", errCount) | ||
| return errors.New(errMsg) |
There was a problem hiding this comment.
For consistency and to be more idiomatic, consider using fmt.Errorf to create the formatted error message directly. A slightly more generic message could also improve clarity.
| errMsg := fmt.Sprintf("failed to remove %d images", errCount) | |
| return errors.New(errMsg) | |
| return fmt.Errorf("encountered errors with %d images", errCount) |
There was a problem hiding this comment.
I saw a lot of places using errors.New(), especially in the files that this PR touches. So with that reasoning I went forward with errors.New.
But I am not against using it :)
both rm and rmi commands now count any failed removals and return an error whenever failed removals are non-zero containers#1770
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 07s |
both rm and rmi commands now count any failed removals and return an error whenever failed removals are non-zero containers#1770 Signed-off-by: om <omhpurohit@gmail.com>
|
Build succeeded. ✔️ unit-test SUCCESS in 2m 10s |
|
@debarshiray would you be able to take a look at this? Sorry for pinging! |
1 participant
This PR aims to fix: #1731
Added error counting to both
rmandrmifunctions, in order to return an error if any removal operation failed. This also ensures the command returns a non-zero exit code for such a case.tested it locally:
