check if lane request's fleet_name is equal to the fleet's fleet_name

[lkw303]

Fixed bug

The Fleet Adapter publishes closed lanes even though the LaneRequest messages were not directed at them. This causes all the fleet adapters to publish a ClosedLanes Message even though the LaneRequest was not meant for them.

Fix applied

Added an if statement to check if fleet_name in LaneRequest is equal to the fleet_name of the fleet. If it is not, return.

[mxgrey]

Thanks for catching this! It looks good to me.

One thing to consider: if the request_msg->fleet_name is blank, then should we apply the request to all fleets? Otherwise to close a lane for all fleets, a user would have to issue a separate lane request to each of them.

[mxgrey]

By the way, we have a "DCO" requirement (helps us avoid lawsuits from patent trolls) which your commit is not currently satisfying. Before we can merge this PR, we'll need you to follow the instructions here: https://github.com/open-rmf/rmf_ros2/pull/95/checks?check_run_id=3259667077

[lkw303]

One thing to consider: if the request_msg->fleet_name is blank, then should we apply the request to all fleets? Otherwise to close a lane for all fleets, a user would have to issue a separate lane request to each of them.

Hi, sorry, Can I clarify, shall I make another commit to add this in?

[mxgrey]

Hi, sorry, Can I clarify, shall I make another commit to add this in?

It looks like you've followed the instructions correctly, so now the DCO check is approved. Thank you!

[mxgrey]

Hi, sorry, Can I clarify, shall I make another commit to add this in?

Oh sorry, I misread the original question here.

I think it's fine either way. If you agree with my suggestion then feel free to create add a commit for it. Otherwise we can leave it as-is and potentially add in the behavior I recommended at another time if anyone requests it.

[mxgrey]

Oh, I apologize for how nit-picky this is, but the code style checker is complaining about trailing whitespace on lines 826 and 828.

Line 826 has 8 trailing spaces while line 828 has 7 trailing spaces i.e.

      if (!connections)
        return;
........⏎
      if (request_msg->fleet_name != fleet_name)
        return;.......⏎

The code style checker won't let the PR pass until those trailing spaces are removed.

[mxgrey]

In case you're using Visual Studio Code I strongly recommend the Strict Whitespace plugin, as well as these settings in your settings.json:

"files.trimTrailingWhitespace": true,
"files.insertFinalNewline": true

[lkw303]

Oh! I see sorry.. my bad. I also added an additional check to see if the fleet_name is empty which you mention previously. Additionally, I made changes to the close_lane node as well. Otherwise if the fleet_name is empty the request would never be fulfilled as the fleet_name from the request will never match the fleet_name of the ClosedLanes messages.

[codecov]

Codecov Report

Merging #95 (8044041) into main (1953ae2) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main      #95      +/-   ##
==========================================
+ Coverage   22.00%   22.03%   +0.03%     
==========================================
  Files         414      621     +207     
  Lines       33080    49629   +16549     
  Branches    16022    24033    +8011     
==========================================
+ Hits         7278    10937    +3659     
- Misses      17994    26959    +8965     
- Partials     7808    11733    +3925     

Flag	Coverage Δ
tests	22.03% <ø> (+0.03%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...leet_adapter/src/rmf_fleet_adapter/agv/Adapter.cpp
...y5jd8xvpm/rmf_ros2/rmf_fleet_adapter/test/main.cpp
...mf_fleet_adapter/test/phases/test_DispenseItem.cpp
...include/rmf_fleet_adapter_python/graph/PyEvent.hpp
...apter/src/rmf_fleet_adapter/jobs/SearchForPath.hpp
...rc/robot_state_aggregator/RobotStateAggregator.cpp
.../rmf_fleet_adapter/rmf_rxcpp/examples/PlanPath.cpp
...adapter/src/rmf_fleet_adapter/agv/TrafficLight.cpp
...et_adapter/src/experimental_lift_watchdog/main.cpp
...apter/src/rmf_fleet_adapter/jobs/SearchForPath.cpp
... and 1025 more

[lkw303]

Hi, sorry but I just noticed that the merging is block as it says that the base branch requires all commits to be signed. Can I check if I should I do a git rebase to sign all my previous commits on my branch?

[mxgrey]

Right, the easiest way I know of for fixing that is to set up the GPG signing using these instructions and then doing a rebase.

Apologies again for all these contribution barriers. Luckily once the GPG signing is set up once, it will work forever (per computer that you use).

[lkw303]

Hi! Just merged with main and have signed off and verified all my previous commits. Hopefully its all good now. Sorry for all the trouble.

[mxgrey]

Thanks for bearing with our CI and review process, and thanks for the contribution!