[Buildbot] fix usage of buildbot outside of repo#1727
[Buildbot] fix usage of buildbot outside of repo#1727tdavidcl merged 1 commit intoShamrock-code:mainfrom
Conversation
|
Thanks @tdavidcl for opening this PR! You can do multiple things directly here: Once the workflow completes a message will appear displaying informations related to the run. Also the PR gets automatically reviewed by gemini, you can: |
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 improves the robustness of the 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. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request refactors the print_buildbot_info function in buildbot/lib/buildbot.py to use subprocess.run for executing git log commands, replacing the less secure and less robust os.popen. It also introduces a try-except block to handle potential errors during git status retrieval, printing a warning if the status cannot be obtained. Review feedback indicates a bug where the git_head variable is duplicated in the output when multiple heads are present. Additionally, it's suggested to update the git diff-index command to also use subprocess.run for consistency and improved error handling, and to refine the exception handling to catch more specific exceptions instead of a generic Exception.
| git_head = ( | ||
| "\033[1;92m" + git_head[0] + "\033[0;0m , \033[1;91m" + git_head[0] + "\033[0;0m" |
There was a problem hiding this comment.
The git_head variable is duplicated in the string formatting when len(git_head) > 1. It appears there might be an intention to display multiple heads or branches, but currently, it prints the first element twice. If there are multiple heads (e.g., HEAD -> main, origin/main), you should display git_head[0] and git_head[1] (or iterate through git_head if more are possible).
| git_head = ( | |
| "\033[1;92m" + git_head[0] + "\033[0;0m , \033[1;91m" + git_head[0] + "\033[0;0m" | |
| git_head = ( | |
| "\033[1;92m" + git_head[0] + "\033[0;0m , \033[1;91m" + git_head[1] + "\033[0;0m" | |
| ) |
| print(" \033[1;93mcommit \033[0;0m: ", git_hash) | ||
| print(" \033[1;36mHEAD \033[0;0m: ", git_head) | ||
| print(" \033[1;31mmodified files\033[0;0m (since last commit):") | ||
| print(os.popen('git diff-index --name-only HEAD -- | sed "s/^/ /g"').read()) |
There was a problem hiding this comment.
The git diff-index command still uses os.popen. For consistency and improved error handling, it's recommended to use subprocess.run for all external command executions, similar to how git log was updated. This also allows for better error handling and avoids shell injection risks associated with os.popen.
| print(os.popen('git diff-index --name-only HEAD -- | sed "s/^/ /g"').read()) | |
| r_diff = subprocess.run( | |
| ["git", "diff-index", "--name-only", "HEAD", "--"], | |
| capture_output=True, | |
| text=True, | |
| check=True, | |
| ) | |
| modified_files = "".join([" " + line for line in r_diff.stdout.splitlines(keepends=True)]) | |
| print(modified_files) |
Workflow reportworkflow report corresponding to commit fe76c84 Pre-commit check reportPre-commit check: ✅ Test pipeline can run. Clang-tidy diff reportNo relevant changes found. You should now go back to your normal life and enjoy a hopefully sunny day while waiting for the review. Doxygen diff with
|
No description provided.