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

Support interdependent PRs #157

Open
rotu opened this issue Apr 11, 2020 · 8 comments · May be fixed by #490
Open

Support interdependent PRs #157

rotu opened this issue Apr 11, 2020 · 8 comments · May be fixed by #490
Labels
enhancement New feature or request

Comments

@rotu
Copy link
Contributor

rotu commented Apr 11, 2020

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. #108
This doesn't fully solve the problem because:

  1. It requires a .repos file to be manually and separately maintained, outside of change management within the repo under test.
  2. 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.
@rotu rotu added the enhancement New feature or request label Apr 11, 2020
@rotu rotu changed the title Support simultaneous PRs Support interdependent PRs Apr 11, 2020
@emersonknapp
Copy link
Contributor

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

@rotu
Copy link
Contributor Author

rotu commented Apr 24, 2020

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
Copy link
Member

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
Copy link
Contributor

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
Copy link
Member

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

@rotu
Copy link
Contributor Author

rotu commented Sep 8, 2020

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

  1. Tell git to fetch pull refs: git config --global --add remote.origin.fetch '+refs/pull/*:refs/remotes/origin/pull/*'
  2. 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
  1. 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
Copy link
Contributor

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
Copy link
Contributor Author

rotu commented Sep 8, 2020

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.

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

Successfully merging a pull request may close this issue.

3 participants