-
Notifications
You must be signed in to change notification settings - Fork 56
Move space_robots from docker repo (space-ros/docker#256) #116
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
Conversation
|
Per the discussion in the technical WG today, I will update the commit message to be more clear that it fixes the space-ros/docker#256 issue, and add additional comments that the history prior to this point is in the docker repo. |
2d61f2b to
2ebf4b5
Compare
|
This is ready for review The only caveat - it currently uses the "moveit2:main" image as a base. Once we release our next release (end of month) in the docker repo, a new image will be generated - "moveit2:latest" and this should be updated to use that image instead. I'll file an issue to track this, if this is approved. |
|
Currently building and will sanity check! In the interim... this is basically a straight copy paste right? In the past I've done something unholy when merging between repos and updating commit messages: space-ros/space-ros#167. I personally think that's overkill, but maybe it's worth updating the README.md to note that it was moved from a different repo? That would at least preserve the link between projects if there were some need to go back in time. |
|
Side note, @EzraBrooks, can you update the repo settings to require approval before merging PRs? That merge button is just sitting there tempting me. |
eholum
left a comment
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.
Did a quick sanity check and found the instructions to be a little off. I haven't been following demo evolution lately so maybe I'm missing something? In any case... would be good to at least tweak the readme so that these work out of the box.
On the plus side they do indeed work out of the box! So much faster to build and run these. Thanks for the work here, @mkhansenbot.
|
I apparently don't have access to this repo's settings. cc @gbiggs |
2ebf4b5 to
cc9c138
Compare
|
Addressed @eholum review feedback, updating the README to use the new launch method of launching GUI and control separately |
cc9c138 to
96c7661
Compare
eholum
left a comment
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.
It does the thing! Thanks for the README updates it just worked out of the box for me.
Moves the
space_robotsdemo from thedockerrepo to this repo (demos). This is dependent on merging space-ros/docker#255 so that this container can pull thespace-ros-moveit2imageThis fixes space-ros/docker#256