Support interdependent PRs

rotu commented 4 years ago

Description

It would be nice to have some support for simultaneous PRs. ROS development is spread across many repositories and it would be nice to be able to support the use case where pull requests are needed to keep things building.

Related Issues

Similar functionality introduced here, where additional repos files can be added. https://github.com/ros-tooling/action-ros-ci/issues/108 This doesn't fully solve the problem because:

It requires a .repos file to be manually and separately maintained, outside of change management within the repo under test.
Adding such a .repos file would break the build after the PR is merged, since the .repos override would "hold back" the project to the pull request, hiding further changes to the repo in question.

Completion Criteria

Allow a list of dependent pull requests and incorporate them into the ROS build under test.

Implementation suggestions:

If any pull requests are open, merge them to the local source before running CI. A failed merge should log an error message and fail the job.
If any pull requests are closed, log a message that they are no longer current.

emersonknapp commented 4 years ago

We want to consider branches on forks for this functionality as well - needs to merge onto the upstream master before building

rotu commented 4 years ago

We want to consider branches on forks for this functionality as well - needs to merge onto the upstream master before building

If I understand you correctly, I think that's what I meant by "If any pull requests are open, merge them to the local source before running CI." Maybe you read my suggestion as "also run the tests of any packages in linked pull requests" which is not what I meant.

christophebedard commented 4 years ago

What do you all think about using some kind of special syntax in pull request descriptions? For example, something inspired by git trailers:

Description of the changes.

Blah blah.

action-ros-ci-dependency: https://github.com/user/repo/pull/123 action-ros-ci-dependency: https://github.com/other-user/other-repo/pull/456

emersonknapp commented 4 years ago

I think that would be pretty clever - I wasn't sure how we would want to approach this, but using special syntax in the PR description might work well.

christophebedard commented 4 years ago

I'll work on it and report back in a little bit!

rotu commented 4 years ago

FYI, @emersonknapp @christophebedard, here's an example of how I implemented something like this in a one-off way: https://github.com/ros2/rmw_cyclonedds/pull/237/files

Tell git to fetch pull refs: git config --global --add remote.origin.fetch '+refs/pull/*:refs/remotes/origin/pull/*'

Create a local.repos file replacing dependencies with their pr branch:

ament/ament_lint:
  type: git
  url: https://github.com/ament/ament_lint.git
  version: pull/268/merge

Pass local.repos file to vcs-repo-file-url to take priority over the ros2 default source

It ain't perfect; pushes to the upstream PR doesn't make the action fire again, and it doesn't handle the case when the upstream PR gets merged. But it does the job enough.

emersonknapp commented 4 years ago

pushes to the upstream PR doesn't make the action fire again

You mean between two interdependent PRs, if you change one of them, the other won't re-run its build? I can't see any obvious way around that using just Actions - I think we'd have to develop a bot or something to manage that - which would require hosting infra etc - probably much too much work. Do you know if any of the CI providers like Travis or Circle have this feature? I haven't really seen it "out in the wild" so I don't have a preexisting notion for the user experience.

rotu commented 4 years ago

You mean between two interdependent PRs, if you change one of them, the other won't re-run its build?

Yes. I'm making the simplifying assumption that the PRs have a consistent order; e.g. pull request A has changes required for pull request B but that A still can pass CI without B. In theory, semver discourages breaking changes without a deprecation version, so this should be the case most of the time. (There might still be a good reason to hold back approval of PR A until package B passes, but CI should at least be able to pass.)

PRs that are truly interdependent (neither can pass CI without the other) are IMO a very bad thing. I think if two changes must be "atomic", that's a sign that the codebases are too tightly coupled and they really belong in a single repo.

I can't see any obvious way around that using just Actions - I think we'd have to develop a bot or something to manage that - which would require hosting infra etc - probably much too much work. Do you know if any of the CI providers like Travis or Circle have this feature? I haven't really seen it "out in the wild" so I don't have a preexisting notion for the user experience.

It does seem like this can be done in a distributed way. Maybe something like a repository_dispatch event or even just a periodic check against related CIs.

I can't speak to Travis or Circle.

ros-tooling / action-ros-ci