-
Notifications
You must be signed in to change notification settings - Fork 440
ci: add trap function to look for core files #15919
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
ci: add trap function to look for core files #15919
Conversation
8b8506e
into
googleapis:prepare-for-v3.0.0
Summary of ChangesHello @scotthart, 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 CI quickstart script to enhance its debugging capabilities by ensuring that core dump files are consistently searched for upon script termination. By introducing a global 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
|
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 trap to ensure core files are searched for when the script exits, for any reason. This is a good improvement for robustness. However, the implementation of the cleanup function has a flaw where it can mask the original exit status of the script, potentially hiding failures in CI. I've suggested a fix to preserve the exit status.
| function cleanup() { | ||
| local exit_status=$? | ||
| io::log_h2 "cleanup on EXIT with exit_status=${exit_status}" | ||
| io::run find . -name '*core*' | ||
| } |
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.
The cleanup function, when triggered by the EXIT trap, will cause the script to exit with the status of the last command it runs (io::run find ...), which is likely 0. This can mask the original exit status of the script if it was failing. To preserve the original exit status, the trap handler should ensure it exits with that status.
| function cleanup() { | |
| local exit_status=$? | |
| io::log_h2 "cleanup on EXIT with exit_status=${exit_status}" | |
| io::run find . -name '*core*' | |
| } | |
| function cleanup() { | |
| local exit_status=$? | |
| io::log_h2 "cleanup on EXIT with exit_status=${exit_status}" | |
| io::run find . -name '*core*' | |
| (exit "${exit_status}") | |
| } |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## prepare-for-v3.0.0 #15919 +/- ##
======================================================
- Coverage 92.85% 92.85% -0.01%
======================================================
Files 2343 2343
Lines 216214 216214
======================================================
- Hits 200773 200770 -3
- Misses 15441 15444 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
No description provided.