Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update Docker build and add CircleCI + CodeCov support #44
Update Docker build and add CircleCI + CodeCov support #44
Changes from 12 commits
0b350f9
826b152
1118615
b6f4a93
ac10b22
95c3036
da182b3
4073586
e93c8b9
c5d41c1
da3173c
7d2b7ac
fc71c65
8a5f875
f42ba65
bfdbc03
4cff4af
bf95820
adc87a7
6d642ca
5940894
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
when? or remove 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.
Is code coverage working now? I was waiting on this until this repo uses something like nav2's cmake changes to enable codecov at build time.
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.
Let's keep this file intact as it is already setup with dockerhub and travis. Perhaps we could move your changes to
.docker/circle-ci/Dockerfile
. Once that is done I am comfortable giving this a +1.On a related note, if you have access to the ros-planning repo, I would recommend pushing your branch to ros-planning/moveit2 so that we can test that everything works with Dockerhub. I did the same thing for my PR #46 and it has been really helpful for iterating quickly.
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.
I've restored the old Dockerfile in 6d642ca , but reminded I had to have new Dockerfile at the root of the project so that the build context may encompass the entire repo that it copies from. I've added a .dockerignore file to mitigate build cache breaks from git fetches or editing of the Dockerfile itself.
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.
moveit/moveit2:master
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.
I omitted the org so user building don't blow away what they may have pulled.
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.
Makes sense. maybe change nav2->moveit2?
Also I really like that the existing dockerfiles have their label in the documentation (eg https://github.com/ros-planning/moveit2/blob/master/.docker/ci/Dockerfile#L1) do you think you can add a simmilar line up top? It could be something like moveit/moveit2:crystal-circle-ci
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.
Rather than have some manual mapping from github branches to dockerhub tags, like
crystal-circle-ci
, I'd recommend we adopt a more programmatic style where release branches and tags on ros-planning/moveit2 github are mapped directly to tag names on ros-planning/moveit2 for CI consumption. There can be procedurally created, while the origin/relation of docker tags to code would be clear and more amenable for CI configs; alluded to in #44 (comment)