Skip to content
This repository has been archived by the owner on Nov 27, 2020. It is now read-only.

DEP status lost on new commit #20

Closed
zetaron opened this issue May 7, 2018 · 10 comments
Closed

DEP status lost on new commit #20

zetaron opened this issue May 7, 2018 · 10 comments

Comments

@zetaron
Copy link

zetaron commented May 7, 2018

I've discovered that DEP currently doesn't act on PRs receiving new commits, which leads to the status being not reported on the most current commit (the one used by github to compute the overall status). That's unfortunate as it prevents me from adding it to the list of required status.

It would be awesome to add DEP to the list of required PR status so that no one can accidentally merge a PR that is awaiting dependencies (eg. because it's based on an other PRs Branch).

@z0al
Copy link
Owner

z0al commented May 7, 2018

Thanks @zetaron for reporting this 👍

I will check the logs and try to figure out what is going on. May you please provide some context? e.g. a PR link?

@z0al
Copy link
Owner

z0al commented May 7, 2018

Oh, just letting you know that this would take a day or so since my laptop isn't working right now hehe

@zetaron
Copy link
Author

zetaron commented May 7, 2018

@ahmed-taj No preassure! Just wanted to bring it to your attention as it seemed odd to me at first :)

If you want I could try to come up with a PR.

@zetaron
Copy link
Author

zetaron commented May 7, 2018

Also I can try to setup a test repository to showcase the issue.

@z0al
Copy link
Owner

z0al commented May 7, 2018

Great! Be my guest ;)

Also, we may need to consider that app might get rate limited since it's hosted on an OSS Zeit plan (https://now.sh) !

@zetaron
Copy link
Author

zetaron commented May 7, 2018

@ahmed-taj good to know, so maybe it's wise to host a small personal version... maybe add a hint to the readme and/or the github installation page?

@z0al
Copy link
Owner

z0al commented May 7, 2018

You're right, I will add this to the installation page shortly!

@z0al
Copy link
Owner

z0al commented May 10, 2018

Hey @zetaron. I've updated the installation page and will do the same on the README soon.

Also I can try to setup a test repository to showcase the issue.

Do you have more info you would like to share?

@zetaron
Copy link
Author

zetaron commented May 12, 2018

@ahmed-taj No I've nothing to add, as soon as the repository is set up I can try to help reproduce this issue for further investigation.

As the repository I originally discovered it is a private one I can't share much more :s sorry.

Thanks for caring so much!

@nesl247
Copy link
Contributor

nesl247 commented Jul 5, 2018

I think this just needs:

robot.on('pull_request.synchronize', update)

here:

dep/index.js

Lines 21 to 25 in 999cc9b

// Re-check on dependency updates
robot.on('issues.closed', update)
robot.on('issues.reopened', update)
robot.on('pull_request.reopened', update)
robot.on('pull_request.closed', update)

@z0al z0al closed this as completed in #26 Jul 7, 2018
z0al pushed a commit that referenced this issue Jul 7, 2018
Fixes #20 by listening to the `pull_request.synchronize` event which is done anytime there is a push.

I didn't test this out myself.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants