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

[IJKL] Enable test_pull_requests for rqt plugin repos that are recently split from meta repos. #14820

Merged
merged 1 commit into from
May 2, 2017

Conversation

130s
Copy link
Member

@130s 130s commented Apr 28, 2017

CI choice should be up to the maintainer of each repository but since these packages are split from unified repos, it makes to me to recommend the default, generic CI option (hopefully most of maintainers will appreciate if CI is already setup).

Jade? Sorry I slacked off there :/

@dirk-thomas
Copy link
Member

Since these repositories do not contain any tests and consist of Python code which isn't being compiled I don't see why those should have PR jobs?

@mikaelarguedas
Copy link
Member

agreed I think a handfull of repos could benefit from it (like https://github.com/ros-visualization/rqt_rviz) but definitely not all of them. I think that leaving it to the maintainers is the best option

@130s
Copy link
Member Author

130s commented Apr 28, 2017

What's the downside of having the default CI in the beginning of new repos do you guys think? If maintainers don't like the buildfarm CI that's :/ but then they can always disable it.

Python package or packages without tests can still benefit from CI; check dependency, grammars etc. Green light on PR is one good sign for merging than not having any validation mechanism.

I don't have a very strong opinion about this PR, but then we should remind the new maintainers about adding CI; understandably maintainers often forget to add CI. I've opened dozens of PRs to add CI config.

@dirk-thomas
Copy link
Member

If maintainers don't care for having CI tests we shouldn't generate them just because we can. They are not "free" - each job costs resources on the master and each build cost resources on the nodes.

@130s
Copy link
Member Author

130s commented Apr 28, 2017

Fair enough. Now I've updated the commit to add test_pull_requests for the only packages that I maintain.contain or rqt_image_view that contains C++ code and you @dirk-thomas maintains.

@dirk-thomas
Copy link
Member

dirk-thomas commented Apr 28, 2017

@130s Please also do the same for Jade.

…ntly split from meta repos. These are what 1) contain C++ code 2) @130s maintain.
@130s
Copy link
Member Author

130s commented May 2, 2017

Yes, done.

@dirk-thomas dirk-thomas merged commit 3f33b29 into ros:master May 2, 2017
@130s 130s changed the title [IKL] Enable test_pull_requests for rqt plugin repos that are recently split from meta repos. [JIKL] Enable test_pull_requests for rqt plugin repos that are recently split from meta repos. May 2, 2017
@130s 130s changed the title [JIKL] Enable test_pull_requests for rqt plugin repos that are recently split from meta repos. [IJKL] Enable test_pull_requests for rqt plugin repos that are recently split from meta repos. May 2, 2017
@130s 130s deleted the rqts/ci branch May 2, 2017 00:45
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.

3 participants