-
Notifications
You must be signed in to change notification settings - Fork 37
Push moveit2 and nav2 images to Docker Hub (issue #254) #255
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
2272cd3 to
50313d6
Compare
0410a73 to
6167895
Compare
|
This is now ready for review |
|
Ignore the fact that it says the |
|
Made one minor change to the |
7ae73a6 to
fca8b1e
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.
Hooray! This will be so great. Sorry for the delay in getting to testing this. I pulled both the moveit2 and nav2 images and just sanity checked them and they seem good!
Had a few minor, minor comments. And can you also update the moveit2 README to have the new image names? There are some leftovers, for instance in moveit:
|
Updated per your review comments @eholum, please review the updates. |
|
Looks good @mkhansenbot! The one last thing I would request is just to squash the PR comment comments into their respective commits. Sorry for the trouble, that's what the last bullet point in https://github.com/space-ros/.github/blob/master/CONTRIBUTING.md#pull-requests references - just keeping the commit message history clean. And then I think we need to change the repo requirements to remove the old CI checks. I don't have permission, can you see those options? https://github.com/space-ros/docker/settings |
|
Thanks Erik, I'll clean up the history a little bit this weekend. As for removing the old CI checks, I think those will be removed by this PR. If not, I think we can merge this and then if they're still hanging around as ghosts, we can address that separately. |
|
Ping me once you've cleaned up the history and I'll make the repo settings change to require the new jobs and not require the old jobs. |
Add space-ros-moveit2 job (issue #254) Push space-ros-nav2 image to Docker Hub (issue #254) Change names to osrf/space-ros-nav2 and osrf/space-ros-moveit2 (issue #254) Only trigger workflows for files that changed (issue #254) Change Nav2 bringup package name (issue #254) Update README files to be more clear (issue #254)
319e1b4 to
101b76c
Compare
|
@eholum - I squashed this down to 2 commits, but listed the main changes under the primary commit, so it's clear what this does. Let me know if this is good. I'd like to get it merged for this release |
Re-requesting approval after addressing the review comment
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.
Woohoo!
@EzraBrooks I think we'll need those CI required checks settings adjusted if you're able to do that?
Pushes the moveit2 and nav2 images to Docker Hub with tags, the same way that the
space-rosimage is pushed. It also pushes to GHCR for use in CI, in the same way as the basespace-roscontainer(s).Edit: These are the Docker Hub repos it pushes to.
https://hub.docker.com/r/osrf/space-ros-moveit2
https://hub.docker.com/r/osrf/space-ros-nav2