-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add diagnostics #2986
Add diagnostics #2986
Conversation
@tonynajjar, please properly fill in PR template in the future. @SteveMacenski, use this instead.
|
@tonynajjar, your PR has failed to build. Please check CI outputs and resolve issues. |
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.
LGTM, just unavoidable linter complaints
void | ||
LifecycleManager::CreateActiveDiagnostic(diagnostic_updater::DiagnosticStatusWrapper & stat) | ||
{ | ||
|
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.
White space that I'm sure CI will complain about - no empty lines at the start or end of the function
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.
Weird, I ran ament_uncrustify
root@10a5941ce854:~# ament_uncrustify /root/ros2_ws/src/navigation2/nav2_lifecycle_manager/src/lifecycle_manager.cpp
No code style divergence in file '/root/ros2_ws/src/navigation2/nav2_lifecycle_manager/src/lifecycle_manager.cpp'
No problems found
Anything else I should run to detect the CI problems you mentioned?
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.
You may have an outdated version 🤷
Test lifecycle also failed: https://app.circleci.com/pipelines/github/ros-planning/navigation2/7390/workflows/720796c3-4123-4fed-a865-8d126264caf3/jobs/26044 that should not be failing, so this might introduce a regression |
I'm not 100% sure but it seems like the nav2_lifecycle_manager tests pass now? I simply fixed the empty lines. Would be nice to see a more detailed summary (or I'm missing where to look):
|
Yeah maybe, I'm going to retrigger CI a few times just to make sure There's a more detailed look in the artifacts tab, sorted by stdout in each package's tests |
Ran 4x, looked related to the linter error, done! |
* add diagnostics * fix * fix
Basic Info
Description of contribution in a few bullet points
Description of documentation updates required from your changes
Future work that may be required in bullet points
For Maintainers: