-
Notifications
You must be signed in to change notification settings - Fork 56
refactor: move external repositories to src/external #374
refactor: move external repositories to src/external #374
Conversation
3805986
to
cf56cfe
Compare
@xmfcx are we not keeping the organization names? |
Replied in autowarefoundation/autoware#374 (comment) |
So, I've restructured the
@esteve @kenji-miyake I've also pushed a commit to show how it'd look. It is provisional, can be reverted too. Please give your opinions on these guidelines/structure. |
@@ -51,15 +51,15 @@ repositories: | |||
url: https://github.com/autowarefoundation/autoware_launch.git | |||
version: main | |||
# sensor_component | |||
sensor_component/sensor_component_description: | |||
sensor_component/external/sensor_component_description: |
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.
sensor_component # I think we should rename this to sensor_driver
@xmfcx I think it's okay, but some repositories like this (sensor_component_description
) aren't driver
.
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.
@kenji-miyake Shouldn't sensor_component_description be under the vehicle
? Since it is about the vehicle specific sensor models and transformations configurations?
And we can name the rest to be drivers
.
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.
Shouldn't sensor_component_description be under the vehicle?
Mm, I feel it's unnatural because it's not completely vehicle-specific. It can be shared between multiple vehicles.
By the way, how about splitting drastic changes in another Issue/PR?
I think it's better to limit the scope of this PR to only vendor -> external
.
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.
By the way, how about splitting drastic changes in another Issue/PR? I think it's better to limit the scope of this PR to only
vendor -> external
.
I agree, I just added them as comments because I didn't want to make those changes under this PR.
type: git | ||
url: https://github.com/tier4/tamagawa_imu_driver.git | ||
version: ros2 | ||
sensor_component/velodyne_vls: | ||
sensor_component/external/velodyne_vls: | ||
type: git | ||
url: https://github.com/tier4/velodyne_vls.git | ||
version: tier4/universe |
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.
sensor_kit # I think we should rename this to sensor_launch and maybe move it under launcher
@xmfcx Mm, I think it's a bit different from sensor_launch
, it's the launcher of the sensor combination.
But we can consider renaming and moving to some other sections.
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.
@kenji-miyake Nevertheless, it is the launcher of the vehicle sensor drivers, I think we should move it under launcher
.
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.
There are two types of launchers. I feel it's better to distinguish them.
- Component launchers (called by Autoware launcher)
- https://github.com/autowarefoundation/autoware.universe/tree/main/launch
- https://github.com/autowarefoundation/sample_sensor_kit_launch
- https://github.com/autowarefoundation/sample_vehicle_launch
- Autoware (the whole system) launcher (calls component launchers)
- https://github.com/autowarefoundation/autoware_launch
- Some customized launchers by the users can be here.
Since I was the one who opened this PR, I can't approve it, but I'm ok with the changes. @xmfcx I'm assuming you're ok with your own changes 😅 so this should be ready to be merged. |
Signed-off-by: Esteve Fernandez <[email protected]>
Signed-off-by: M. Fatih Cırıt <[email protected]>
Signed-off-by: M. Fatih Cırıt <[email protected]>
Co-authored-by: Kenji Miyake <[email protected]>
7b93463
to
b1b6c22
Compare
Signed-off-by: Esteve Fernandez [email protected]
Description
Move external repositories to
src/external
Pre-review checklist for the PR author
The PR author must check the checkboxes below when creating the PR.
In-review checklist for the PR reviewers
The PR reviewers must check the checkboxes below before approval.
Post-review checklist for the PR author
The PR author must check the checkboxes below before merging.
After all checkboxes are checked, anyone who has write access can merge the PR.