Comments (26)
@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
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
from dep.
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.
from dep.
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.
from dep.
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. π
from dep.
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 ;)
from dep.
@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?
from dep.
Yeah, thatβs all I really need. Thanks so much.
from dep.
@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?
from dep.
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 :)
from dep.
@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?
from dep.
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
from dep.
@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 onrsarky/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.
from dep.
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?
from dep.
@rsarky correct.
from dep.
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?
from dep.
@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
from dep.
So on any of the above actions we poll the external repo dependncies for their status right?
LGTM π
from dep.
So on any of the above actions, we poll the external repo dependencies for their status right?
@rsarky yup
from dep.
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.
from dep.
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.
from dep.
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 ?
from dep.
A pull request was opened for that reason but was closed
Can you refer to the PR?
from dep.
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
from dep.
@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.
from dep.
@SouhaibBenFarhat was you able to find the solution for linking cross-repo PRs?
from dep.
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.
from dep.
Related Issues (20)
- Multiple commands in the same PR are not recognized HOT 1
- Make DEP a yellow waiting dot instead of a red cross. HOT 11
- Post a green status when there are no dependencies. HOT 7
- [RFC] Handle full PR URLs and not only #NUMBER HOT 3
- Version 10 of node.js has been released
- DEP status lost on new commit HOT 10
- Support for explicitly marking it as ok to merge HOT 5
- Waiting for status to be reported HOT 1
- feature request: extend functionality to work with issues HOT 4
- I've sent a pr to remove contributor section on probot HOT 1
- Don't require a / in the beginning HOT 1
- Status not changing to pending HOT 2
- Configurable regexes HOT 2
- the DEP step is hanging out... waiting for something that will never happen.. HOT 7
- Bot occasionally stops working HOT 6
- Not working HOT 6
- Update base when dependency is merged HOT 25
- Prevent dependency of a PR on itself HOT 3
- Allow commands also within PR description HOT 4
- Status is not updated when command is edited or removed HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
π Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. πππ
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google β€οΈ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from dep.