Skip to content
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

Update Maintainers of MoveIt package #697

Merged
merged 6 commits into from
Nov 15, 2021
Merged

Update Maintainers of MoveIt package #697

merged 6 commits into from
Nov 15, 2021

Conversation

davetcoleman
Copy link
Member

I'm getting a lot of noisy emails from Jenkins such as:

Build failed in Jenkins: Fdev__moveit__ubuntu_focal_amd64 #148

But I do not have bandwidth for this anymore. I'm also guessing @mikeferguson @IanTheEngineer @130s are not looking at this either, since we have not seen them in a long time.

I'd like to instead replace us with @henningkayser @tylerjw who are very actively involved.

I've also added myself as an author of MoveIt, which is mostly an ego thing that I'm fine removing also if it is judged my involvement at Willow Garage since before MoveIt was released as beta is not enough. I'm cool either way but this is clearly my preference.

Copy link
Contributor

@130s 130s left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

If this would stop the notification from jenkins that'll be great too (my impression from the last I tried to do that a few years ago, updating package.xml may have not been enough though).

@mikeferguson
Copy link
Contributor

Yeah - that fetchrobotics email hasn't worked in about 4 years now...

@mikeferguson
Copy link
Contributor

If this would stop the notification from jenkins that'll be great too (my impression from the last I tried to do that a few years ago, updating package.xml may have not been enough though).

If I recall correctly, you have to do a release as well to get the buildfarm to fully update

@henningkayser
Copy link
Member

Makes sense to me. Shouldn't this be changed for the other package.xml files as well?

@codecov
Copy link

codecov bot commented Sep 22, 2021

Codecov Report

Merging #697 (f9838b8) into main (eaa68f2) will decrease coverage by 1.19%.
The diff coverage is n/a.

❗ Current head f9838b8 differs from pull request most recent head 47d3a12. Consider uploading reports for the commit 47d3a12 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main     #697      +/-   ##
==========================================
- Coverage   55.41%   54.23%   -1.18%     
==========================================
  Files         196      192       -4     
  Lines       21402    20224    -1178     
==========================================
- Hits        11858    10966     -892     
+ Misses       9544     9258     -286     
Impacted Files Coverage Δ
...rajectory_processing/src/ruckig_traj_smoothing.cpp 0.00% <0.00%> (-80.61%) ⬇️
...eit_core/robot_trajectory/src/robot_trajectory.cpp 23.13% <0.00%> (-31.02%) ⬇️
...include/moveit/robot_trajectory/robot_trajectory.h 85.72% <0.00%> (-10.38%) ⬇️
..._core/collision_detection/src/collision_common.cpp 42.86% <0.00%> (-7.14%) ⬇️
...sion_plugin_loader/src/collision_plugin_loader.cpp 30.77% <0.00%> (-6.44%) ⬇️
...veit_servo/include/moveit_servo/servo_parameters.h 69.24% <0.00%> (-5.76%) ⬇️
...space/constrained_planning_state_space_factory.cpp 66.67% <0.00%> (-4.76%) ⬇️
...bullet_integration/bullet_discrete_bvh_manager.cpp 50.00% <0.00%> (-3.84%) ⬇️
...lude/moveit/collision_detection/collision_common.h 76.75% <0.00%> (-3.25%) ⬇️
moveit_core/robot_state/src/attached_body.cpp 25.93% <0.00%> (-2.88%) ⬇️
... and 142 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 542c5f7...47d3a12. Read the comment docs.

Copy link
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me. Shouldn't this be changed for the other package.xml files as well?

Yes, but it depends on Dave's preferences of course. @davetcoleman Hope you got the cycles to clean these entries consistently across all packages. You probably want to have similar changes in the moveit repository as well?

It makes sense to add you as an author in the meta-package. You are listed as author of MSA already which is a part of the stack.

@davetcoleman
Copy link
Member Author

@v4hn can't tell if you're being sarcastic?

It would be great to have all the package.xml files updated. I did this edit through the Github webpage editor to address the specific issue of emails I was getting, it's a bit tougher for me to edit other instances as I don't have the code checked out.

@v4hn
Copy link
Contributor

v4hn commented Nov 4, 2021

can't tell if you're being sarcastic?

I'm not. it's entirely your decision on where you want to be listed.
I'm surprised this is still not merged.

It would be great if you could go through the packages systematically and adapt the maintainers as you did here.
git clone + grep takes something like 5 minutes.
If you can't do it, I'm also ok with merging this standalone.

* Remove Dave's maintainer entries with with Tyler,Henning
* Add Dave as author to moveit_core, moveit_ros
* Add missing authors to moveit_common, run_move_group, run_moeit_cpp
* Remove entries of inactive maintainers
@henningkayser
Copy link
Member

henningkayser commented Nov 4, 2021

I pushed a commit that
* Removes Dave's maintainer entries and replaces them with @tylerjw ,Henning
* Adds Dave as author to moveit_core, moveit_ros
* Adds missing authors to moveit_common, run_move_group, run_moeit_cpp
* Removes entries of inactive maintainers

I would still like to make the entries more accurate from PickNik's side (add more of @JafarAbdi , @AndyZe , hopefully soon @vatanaksoytezer ), but we can still update these later.

@henningkayser
Copy link
Member

henningkayser commented Nov 4, 2021

xmllint wasn't happy with the changes. so I disabled the xml schema for the "broken" files in 5904793 for better diff readability.
#779 adds the ROS format 3 schema and fixes it for all package.xml files. The failing rolling-ci job is a caching issue.

@tylerjw tylerjw merged commit 6cdc62a into main Nov 15, 2021
@delete-merged-branch delete-merged-branch bot deleted the pr-update-maintainers branch November 15, 2021 15:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants