Support for cross-repository dependencies

[nesl247]

It would be awesome if this would work across repositories. That's actually the reason we looked into this bot.

[z0al]

Hey @nesl247 , thank you for opening this!

Yeah, working with dependencies across repositories sounds cool. Technically, at least for the automatic status update, it's limited to the repositories the bot has gained access once installed! Would you need to include dependencies from "external" repositories?

I will take a look to see what I can do ;)

[z0al]

@nesl247 it's not possible for me to support cross-repo deps for all repositories.

I may only support repositories allowed per installation e.g. if you installed the app on owner and allowed the app to access owner/repo-1 and owner/repo-2 it will only work for those allowed repos and not e.g. owner/repo-3 or webpack/webpack. In that case, would it still be useful or good enough for you?

[nesl247]

Yeah, that’s all I really need. Thanks so much.

[laughedelic]

@ahmed-taj could you clarify why it's not possible? It seems that the check here

https://github.com/ahmed-taj/dep/blob/da4aed8db829e4d3c98db282ff27c0d09eb7c40c/lib/check.js#L17

can access issues of any public repository. Or am I missing something?

[z0al]

Good question @laughedelic!

The app mainly listens to issue events(opened,closed), when an issue is closed we check if it's a dependecy of an open PR and update that PR's state accordingly!

The only - official - way to get issue events is through subscribing to GitHub webhooks. But you can't unless the owner of that repository installed the app and authorized access to it. In short, we won't get events when those public repository (who their owners haven't installed our little app - 99.9% of repos) issues updated.

And yes, we also run checks on a PR if new commits have been pushed to it, but that is just a double-check in case some webhooks failed to be delivered to the app due the current infrastructure limitations (free heroku plan).

I hope it's clear now :)

[laughedelic]

@ahmed-taj thanks for the explanation, I understand now where the limitation comes from: it's about event updates, not the state checks. But what if look at it from a different angle:

we also run checks on a PR if new commits have been pushed to it

So if you already check PR's status whenever it's updated, you could check any external issue-dependencies. Add a listener for any kind of PR update (comments, reviews, pushes, labels, etc.) and check dependencies states (regardless of whether they are external or not). What do you think about this approach?

[rsarky]

So if you already check PR's status whenever it's updated, you could check any external issue-dependencies. Add a listener for any kind of PR update (comments, reviews, pushes, labels, etc.) and check dependencies states (regardless of whether they are external or not). What do you think about this approach?

I dont this will work because an event will only be generated when a webhook is subscribed to.
And that will only happen when the app is installed in the repository.
So an external dependency where the app has not been installed is impossible to handle IMO

[laughedelic]

@rsarky you don't need to install the bot on other repos to check their issues (assuming repos are public). Here's a simple example:

• I have a repo laughedelic/foo where I installed this bot
• now I open an issue #42 in it and write a comment: Depends on rsarky/hello-world#1 (external dependency)

What does the bot do:

• listens on all issue (or pull-request) related events in my repo (in particular laughedelic/foo#42)
• I post a comment, push commits or do any other change to the issue
• the bot sees that #42 depends on rsarky/hello-world#1 and checks if it's open or closed
• changes the status of #42 accordingly

Notice that it doesn't need to react on events from rsarky/hello-world#1. It needs to change the status of a dependent issue, so it's enough if it reacts only on the events from it.

I agree that it's less efficient, but I'm just showing that it's possible to handle external dependencies.

[rsarky]

So what you are proposing is instead of subscribing to events from an external repo we passively check if the external dependencies are open or closed?

[laughedelic]

@rsarky correct.

[rsarky]

Hmmm.
I think it is a good idea although I have some questions.
How often will you poll the external repo for changes and when will you poll?

[z0al]

@rsarky currently the app mainly listens for pull request open/close events:

https://github.com/ahmed-taj/dep/blob/4ec93bcf2429553bc5b960ab212161bd75ad1f6f/index.js#L17-L25

If I understand @laughedelic 's proposal correctly. We will need to listen to more PR related events e.g. pull_request_review, pull_request_review_comment ..etc. in order to re-check and update PR accordingly ASAP.

Then we will need to modify our matching regex to support external repo referencing

https://github.com/ahmed-taj/dep/blob/4ec93bcf2429553bc5b960ab212161bd75ad1f6f/lib/helpers/match.js#L1

To somthing like this (not tested):

const pattern = /depend(?:s|ed)?(?:[ \t]+(?:up)?on)?[ \t]+((?:[\w-.]+\/[\w-.]+)?#\d+)/gi

By the way, technically, the app actually receives some other PR events like assigned, unassigned, labeled, unlabeled, edited, and synchronized, but it ignores them as it weren't relevant.

How often will you poll the external repo for changes

Depends on how often that PR changes

[laughedelic]

@ahmed-taj right. I'm glad that I could explain my idea in the end.

Technically you can subscribe to all events from pull-requests: robot.on('pull_request', ...) (btw, the bot has those permissions already). Probably it's too much. I would consider at least these actions relevant to trigger update:

• pull_request.opened
• pull_request.edited
• pull_request.reopened
• pull_request.synchronize
• pull_request.review_requested

[rsarky]

So on any of the above actions we poll the external repo dependncies for their status right?
LGTM 👍

[z0al]

So on any of the above actions, we poll the external repo dependencies for their status right?

@rsarky yup

[ryanhiebert]

We're trying to simulate updates that are triggered by an external repository by checking with whatever events we get from the current repository. I think it makes sense, if we're going to implement a hack like this, we should do it on any event that won't be recursive (if there's an event for "update a status", then we shouldn't do the check for that). And it should check all pull requests for each of those events, so that it doesn't even need to be related to the pull request in question in order to update the status for the external repository.

This is a not insignificant overhead. But I'll let @ahmed-taj decide if the trade-offs are worthwhile.

[charpeni]

Thumbs up, that's exactly what we're looking for react-native and react-native-website.

Some pull requests in the documentation depends on pull request in the core repo and at the moment we handle this manually. I think this bot could be a great improvement in the process flow.

Additionally, if the bot could also comment in the PRs to notify us that would be great. I can submit a PR with that once the cross-repository will be available.

Keep us up-to-date.

[z0al]

Thank you all for sharing your thoughts, I really appreciate it 🙌

@charpeni consider this real example: facebook/react-native-website#401 which depends on facebook/react-native#19627

None of these events would actually trigger an update (cc @laughedelic ):

• pull_request.opened
• pull_request.edited
• pull_request.reopened
• pull_request.synchronize
• pull_request.review_requested

For the example above, I don't see any event will ever be triggered on that specific PR.

While it's possible to listen to some more events (e.g. push, issues, ...etc), the more we add the more likely the app would hit GitHub API limits (and resources limit - at least for the hosted version at https://probot-dep.now.sh).
Currently, the app listens to these events to update deps:

• issues.opened
• issues.reopened
• pull_request.reopened
• pull_request.closed
• pull_request.synchronize

I don't know but, what if we only listened to push event? I can't determine if that would be enough/too much for our purpose.

[bobvanderlinden]

It might make sense to have a way to manually reevaluate checks. Might be able to make use of checkrun actions https://developer.github.com/v3/checks/runs/#parameters.

[SouhaibBenFarhat]

Hi guys, any updates about this enhancement, I tested this dep app and looks exactly what we need currently in our company, the only problem is that dep doesn't support cross repository dependencies, seems like the fix is just to update the regex matching pattern and extend it to match pull request urls as well.
A pull request was opened for that reason but was closed, any new updates about this ?

[bobvanderlinden]

A pull request was opened for that reason but was closed

Can you refer to the PR?

[SouhaibBenFarhat]

A pull request was opened for that reason but was closed

Can you refer to the PR?

Thanks for your interaction, I see sign of life here 😄
this is the pull request.
#6

[z0al]

@SouhaibBenFarhat that PR was not actually related.

Regarding this. I'm afraid I don't have much time to work on this anytime soon. Please feel free to submit a pull request.

[SouhaibBenFarhat]

I will try to investigate and see how things works with probot and octokit, try to implement something in the next few days, Thanks for your interaction. You did a very nice job. 👍

[frixatrix]

@SouhaibBenFarhat was you able to find the solution for linking cross-repo PRs?

[z0al]

Hey everyone I hope you are doing well.

I was able to build a new GitHub action that replaces this bot.

The new action is more robust and has a few more features like customizing the keywords, label, and handling of cross-repository dependencies 🎉

Please check it out at https://github.com/z0al/dependent-issues

This bot is now deprecated and I will be archiving this repository in the upcoming days. Thanks.