Skip to content
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

BEFORE_DOCKER_SCRIPT #42

Merged
merged 2 commits into from
Jan 24, 2019
Merged

Conversation

rhaschke
Copy link
Contributor

(Optionally) allow execution of a script before starting the docker container.
Addresses moveit/moveit#1320 (comment).

@davetcoleman
Copy link
Member

This feels clunky, what do you think of instead running the IKFast test similar to how clang-format is run? In the Travis test matrix we could have:

 - ROS_DISTRO=melodic  ROS_REPO=ros  TEST=ikfast                                                             

Or maybe to help me better understand this new feature, could you add a small section to the moveit_ci README explaining this new function?

@rhaschke
Copy link
Contributor Author

rhaschke commented Jan 24, 2019

What do you think of instead running the IKFast test similar to how clang-format is run?

That's not possible. The intended script doesn't run the test (as in clang-format), but it creates the ikfast plugins from the templates. The generated plugins then need to be build as part of the catkin workspace, such that they will be found as ROS packages by the prepared kinematics tests.

@davetcoleman I added the requested README comment.

@rhaschke
Copy link
Contributor Author

@davetcoleman Was there a special reason that you approved but didn't merged?

@rhaschke rhaschke merged commit 81c60af into moveit:master Jan 24, 2019
@rhaschke rhaschke deleted the before-docker-script branch January 24, 2019 08:37
@davetcoleman
Copy link
Member

I wasn't super excited about it (see my feedback) so wanted to give anyone else a chance to review, or for you to have a second thought.

@rhaschke
Copy link
Contributor Author

The BEFORE_DOCKER_SCRIPT is definitely needed to run another docker container before, which is needed to test the ikfast-generation pipeline. So know 2nd thought required on my side ;-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants