bad PR / commit practice

[rhaschke]

Obviously, since a while, you guys at PickNik only file PRs against the PickNik fork of this repo, but then fast-forward the ros-planning repo as well (probably this was done only recently). This leads to commits, here, in the main repo that refer to misleading PR IDs, e.g. 85814db referring to #55, which is completely unrelated (because they actually refer to PRs in PickNik's fork).
Please stop doing so and instead file PRs directly against the main repo at ros-planning.

The second drawback of this practice is, that you didn't notice that the moveit_tutorials package is broken in Travis since two months: https://travis-ci.org/ros-planning/moveit_tutorials/branches.
Please fix this asap. Since @davetcoleman added the tutorials package to the travis build of the main moveit repo in melodic, this is now failing too.

[rhaschke]

Obviously the failing Travis is due to a change of the underlying Travis installation, now providing a more recent ruby version than the one we manually install. I'm experimenting with a fix in #167.

[rhaschke]

Having fixed Travis' ruby issue, there pop up dozens of broken link warnings: https://travis-ci.org/ros-planning/moveit_tutorials/builds/379740334. Please fix them.

[davetcoleman]

@rhaschke let's keep this dialog polite, feels very accusatory. We just put a lot of effort into greatly improving the tutorials and issues always occur when making lots of changes.

• Re: PR numbers, the client (Franka) wanted some over site and review before we went "live" so we had to work in a separate Github organization. Plenty of companies/labs develop internally before releasing open source, I don't think you can ask that to change. I also don't think the broken PR links are that big a deal. Do you have any recommendations for avoiding this beyond just requesting we commit directly to ros-planning?

• The broken Travis build is not related with our PickNikRobotics fork so i don't think should be conflated with "bad PR practice". Something broke on CI unrelated to us (ruby?) and yes, should be fixed by whomever feels motivated to do so.

• The broken link warnings on Travis are because the ROS buildfarm has not built the new tutorials yet. We have already been looking into this issue - it appears ROS build only runs every ~2 weeks now but we're not sure why: http://build.ros.org/job/Kdoc__moveit_tutorials__ubuntu_xenial_amd64/lastBuild/
  @mlautman is going to contact Tully

• Thanks for fixing the Ruby issues!

[rhaschke]

• @davetcoleman I'm sorry if my English was offending. That wasn't my intention at all. I was just very confused, when I tried to figure out the reason for the Travis failure of moveit_tutorials and couldn't find corresponding pull requests to recent commits, although this was a guideline you strongly insisted in in the past.
• In hindsight, I understand your motivation. However, instead of simply fast-forwarding this repo from your local one, a better approach might have been to explicitly merge. In this fashion, the commit history would have become more obvious.
• Indeed, the broken Travis was not related to your recent commits. However, having Travis not enabled for your local company repo, you maybe didn't noticed these issues over the past two months. At least, I didn't.

[davetcoleman]

instead of simply fast-forwarding this repo from your local one, a better approach might have been to explicitly merge.

I did a rebase-and-merge... I'm afraid I don't understand the difference enough but will try to research it more next time

[rhaschke]

While a rebase tries to maintain a linear commit history, an explicit merge commit allows to recognize that a bunch of commits was merged in from somewhere else and it also indicates from where.