search

zivid / zivid-ros

Official ROS driver for Zivid 3D cameras
BSD 3-Clause "New" or "Revised" License
55 stars 43 forks source link

Porting CI from azure to GitHub actions #52

Closed gnpinkert closed 2 years ago

gnpinkert commented 2 years ago

Porting the Azure CI to GitHub actions. Pre-work for adding a teams notification for the ZIVID-ROS pipeline

gnpinkert commented 2 years ago

That was quick, generally, it looks good, but I have some comments:

It is a bit surprising you need to change shell scrips when just porting the workflow. Are these changes needed for the port or nice to have cleanups? You should put them on a separate commit so it can be documented why this is included in the PR.

You don't need to change shell scripts, I didn't see the need for another shell script which just checks that arguments exist before starting the docker run (which are included in the matrix's of each run). If the arguments don't exist, then the script is going to fail anyway. I don't have a problem running that script instead if you prefer. If not, then I can do the separate commit.

nedrebo commented 2 years ago

That was quick, generally, it looks good, but I have some comments: It is a bit surprising you need to change shell scrips when just porting the workflow. Are these changes needed for the port or nice to have cleanups? You should put them on a separate commit so it can be documented why this is included in the PR.

You don't need to change shell scripts, I didn't see the need for another shell script which just checks that arguments exist before starting the docker run (which are included in the matrix's of each run). If the arguments don't exist, then the script is going to fail anyway. I don't have a problem running that script instead if you prefer. If not, then I can do the separate commit.

Do what you think is best, my main feedback was that you should document the reason for the change. Especially when it seems unrelated to the main topic of the PR.

nedrebo commented 2 years ago

Seems like you have enough reviewers. I'm signing off from the review, don't consider me a blocker for merge.

eskaur commented 2 years ago

@gnpinkert : Your commit-messages are still filled with fixup-notes btw.