dlang / dlang-bot Goto Github PK

It seems like we always try to merge with an old sha?

Response if sha was provided and pull request head did not match

Status: 409 Conflict
{
  "message": "Head branch was modified. Review and try the merge again.",
  "documentation_url": "https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button"
}

See also: dlang/phobos#5466 (comment)

Nobody will notice anyways and it is good to preserve the history.

From @WebDrake

I would suggest that some CI are more important than others -- so e.g. style CI should not block moving to 'needs review', but test-suite failure should probably see the tag set to 'needs work' (maybe with a friendly message about how to address the test-suite failure the first time it happens).

UI-wise, I like the idea of having the merge toggle in github. But I'm a bit concerned about accidentally merging because you clicked the wrong label. Definitely there needs to be a delay in merging (probably at least 20 seconds).

You could also accidentally hit the "green" merge button - it would be immediate as well.

That's not next to the "allocator" label button 😉
Hm... a crude idea on that -- the labels seem listed alphabetically, you could make the label "ZZZ auto-merge" so they are always at the bottom.

or e.g. {automerge}

CC @schveiugy (from dlang/phobos#4967)

@MartinNowak I am not sure how much a bot can help you with the release process - in other words: what is taking up time?

Anyhow, here are a couple of ideas that come to my mind:

• automatically open a PR to "master" from "stable" after a merged regression fix (or every X days)
• update released version (Wikipedia, DWiki) - though I guess that's better than via a one-off script
• automatically warn on PRs with a linked milestone when only X days remain until the targeted date

While a contributor can't set labels himself, he could do so via the PR title. Some are already commonly used:

• [Trivial] -> add "Trivial" label

To avoid any security issues the bot would:

• only add labels not remove them
• only used a fixed set of label

Proposed list (for the beginning):

• Trivial
• Doc
• WIP

Multiple labels could be provided with a comma, e.g. "[Trivial, Doc] A fancy title" though I doubt that this will be a common case.

https://blog.travis-ci.com/2017-03-22-introducing-auto-cancellation

This is one of the top 3 items that newcomers tend to forget.

We could have the bot ignore a couple of prefixes e.g. [Trivial], [Style].

So my idea would be to send a comment on all PRs (except for explicitly ignored one) and saying something like:

Hi @awesomedcontributor.
Thanks a lot for your contribution! We have seen that you have neither included a bug reference nor changelog entry.
In case you forgot, please see our [documentation](https://wiki.dlang.org/Contributing_to_Phobos).

Implementation would be simple as it's just the default case:

• no issue found
• title doesn't start with X
• diff doesn't contain files in changelog/

I am just not sure whether this would be useful in general or too spamful due to false positives.

Only done on active board for now.

Inspired by dlang/phobos#5529 (comment) and previous discussions.

To allow everyone from the D community to review PRs they are interested in, it's good to allow them to be open for at least 24 hours. This gives everyone from around the globe and any timezone a chance to have a look at it before it's merged.

PRs labeled as "trivial" could be exempted from this rule; if we find it necessary, a new "urgent" label could be added as well. However, manual merging once required CIs are green is still an option to all users with push access.

Not that much related to dlang-bot, but just found an interesting Ruby library.
Would be interesting to have sth. similar for D.
GitHub - vcr/vcr: Record your test suite's HTTP interactions and replay them during future test runs for fast, deterministic, accurate tests.

we should immediately run the auto-merge checker if we receive an approve event, e.g.

dlang/dlang.org#1775

e.g. dlang/dmd#6589

With #57 we already query Bugzilla for this information, so once pulled, this would be trivial to achieve.

e.g. the D-man

If you want to pick a special one -> https://github.com/wilzbach/d-mans
Here are some ideas:

The more detailed option of #84 is needed.

Problem: the CodeCov failures happen to often and everytime they mark a PR with a fat "red cross".
Proposed solution: disable CodeCov CI reports, let the bot query the status results manually and display them in the bot's comment.

I don't know how "fast" CodeCov is in interpreting the results, but we could try the following:

whenever there's a CircleCi event wait X minutes and query CodeCov, on failure wait 2*X minutes and query CodeCov again

Only relevant for repos which require approval, e.g. {phobos, druntime, dlang.org}. Currently, dmd does not.

Apparently it's possible to provide custom hooks for Bugzilla as well (e.g. https://www.bugzilla.org/docs/4.4/en/html/api/Bugzilla/Hook.html), but it seems a bit more complicated

Otherwise a simple cron job that queries the bugzilla API for new regression every X hours should work as well.

http://forum.dlang.org/post/[email protected]

• Maybe two days instead of one?
• Mail to Dlang-internal?

It seems to be common for newcomers to use "short" git commit messages that don't reflect the issue.
A fix for this could be

1. to check the PR title for issue references too as other people could change the title
2. write them that if a title contains the word issue or fix that the syntax is wrong

dlang/phobos#4702
dlang/phobos#4740

For example the tools repo got broken with the 2.072 release due, so it would be nice to ensure everything is working.
Idk if retriggering the last build would work because afaik then only the person who merged the PR gets notified (this could be solved by posting to the mailing list) and if it's daily it could get spammy in case of an error (imho that's wanted).

We could use on of these ping monitors / free cron job services out there and run a special route once a day (or twice).

From @CyberShadow at dlang/dlang.org#1767 (comment)

How about checking if https://api.github.com/repos/dlang/dlang.org/commits?author=wilzbach is non-empty? Then contributors would also start getting the shorter message once their first PR is accepted.
...
?per_page=100 but either way that solution doesn't scale.

I think this boils down to the question:

Do we want to hide the hello world message from organization public members or even contributors?

Imho: it does make sense to provide these instructions for contributors as well and it doesn't cost us anything ...

We should check the result of the labels API before sending an API request to GH to avoid this:

From dlang/phobos#5511

(or at least reduce it to a single CI)

I prefer the auto-merge system actually posting the message that auto merge has been toggled on, because that goes to my inbox.

reported by @schveiguy

The idea is simple: old PRs require a lot of manual pinging and if the author is gone, it usually ends up being closed after a couple of pings.

The bot could do the following:

• if a PR has had no activity with X days -> ping and warn for auto-close
• do so a second after X days
• after another X days automatically close the PR

For the commenting another GitHub account can be used, so that it doesn't conflict with the ones from the bot and is easier to identify in emails.
The ping could be done once a day (e. g. as a cron job that calls the Heroku URL to workaround the auto-sleep problem. For authentication to avoid incorrect use a simple token could be used that is stored as ENV and SSL URL)

Might turn out interesting for us as well.

• ansibullbot/ISSUE_HELP.md at master · ansible/ansibullbot

For example this one https://trello.com/c/gaz5dUak/267-issue-16652-reg-2-071-returned-rvalue-destroyed-too-early with the current title Issue 16652 – [Reg 2.071] returned rvalue destroyed too early.

Especially at Phobos it's a lot of work to point out common mistakes and I think we should automate this. An incomplete list of common mistakes

1. Bugfix without unittest
2. Bugfix without Bugzilla reference in correct syntax
3. Style / whitespace violations

• not everything is covered by make -f posix.mak style
  4) ...

Another big problem is that test failures are often hard to interpret for newcomers. Maybe we could have a simple heuristic that comments if there has been a failure and the user has submitted less than five PRs to Phobos with helpful information about how to deal with CI errors?

• add a main method
• add a CLI flag to call the cron job --run
• configure the heroku addon
  https://devcenter.heroku.com/articles/scheduler

updatedAt of the PR gets updated with adding a label and we currently just look at the last comment.
In some cases looking at updatedAt of the commits would be helpful as well:

https://api.github.com/repos/dlang/phobos/pulls/5004

Would save a couple of clicks if we could merge approve and adding the auto-merge label into a single link or so.
dlang/phobos#5382 (comment)

Our first experiment with the mention bot didn't work that well, but we should look into learning from it and rebooting the idea.

Some learned lessons:

• use interpolation of the time changed (especially braddr was mentioned a lot because I think he initialized the git repo back in the old days)
• wait for X days until assigning reviewers as someone might have already picked up the PR or even merged it
• always assign more than one reviewer
• use the new review assign feature (I think there's no API yet)
• don't mention reviewers that don't have write access
• include the last activity on a dlang repo in the evaluation (workflow tend to differs, but generally people have their "active" phases)
• don't post a separate comment

The daily cron seems to work nicely as can be seen with the auto-labelled issues:

• https://github.com/dlang/phobos/issues?q=is%3Aopen+is%3Apr+label%3Astalled
• https://github.com/dlang/phobos/pulls?q=is%3Aopen+is%3Apr+label%3A%22needs+rebase%22
• https://github.com/dlang/phobos/pulls?q=is%3Aopen+is%3Apr+label%3A%22needs+work%22

However, after an yet unknown issue, it segfaults.
This isn't a serious problems as it will just stop to label prs, but we should definitely investigate this.

See dlang/phobos#5519 which contains 3 dlang-bot added the Bug Fix label ... messages.

Atm the biggest downside imho of the "auto-merge" label system is that the auto-tester doesn't know anything about this status and thus doesn't prioritize "auto-merge" PRs.

@braddr atm the "auto-merge" feature is rather simple and done via labels as follows:

• Only users with write permissions can add labels (this depends on GH not changing their permissions model in the near future)
• There are two labels: auto-merge and auto-merge-squash (the latter will perform a squashed merge)
• Dlang-bot has write-access and will do the merge once all required CI status provider pass

@braddr: Is there an easy way to let "labelled" PRs also be prioritized?
The simplest idea that comes to my mind is to let the Dlang-Bot call the auto-toggle API (https://auto-tester.puremagic.com/addv2/toggle_auto_merge) (and probably disable the GH API request for merge on the auto-tester as (1) the dlang-bot writes the name of the author as part of the merge commit and (2) it probably will fail a lot anyways due to the required CI status checks).
Now I saw that you use a CSRF token for your API. Of course we could scrape the page and thus get a valid token, but I guess you have a better way to let the Bot toggle the auto-merge prioritization?

I just looked through the log on heroku and it would be quite nice to print all debug statements as well. With the fancy logging addons at Heroku, we can filter them out if needed anyways, but we can't put them in ;-)

Happens for the dlang-bot repo which doesn't get many PR status messages, thus an auto-merge will never happen in case the lastFullPRCheck was too recent before a new PR.
We could make sure to always schedule have a pending auto-merge PR check when receiving a success build status, e.g. http://vibed.org/api/vibe.core.core/Timer.

The idea is to compile a list of interesting or easy to review PRs with a better exposure (D forum, D's weekly newsletter).

The easiest way to do so would be to have a special label like "easy review".

Moreover, something similar could be done with the bug list.

@andralex proposed a "torrent" approach for reviewing PRs a while ago:

In the BitTorrent protocol,
you only get bits if you offer bits. In the envisioned "review torrent"
protocol, the more PRs you review, the more likely your PR is to get
reviewed. Do you think this could be supported by tooling?

I am not sure yet how tooling could help, but I am putting this here so that it's not forgotten and has a better visibility. Maybe someone else has a good idea?

Could be helpful:

(seen here)

However, I am not sure how noisy this would be become

Estimate possible approaches and effort first.

Do we need user OAuth tokens to merge on their behalf?

Could we live w/o the merge author in git and just use the dlang-bot account?

What buttons to use? I think labels were proposed b/c they're already only editable by maintainers.

Should be pretty trivial and could help a bit to filter for pending fixes prior the merge window.

It might be more complicated to automatically remove the label and I guess this won't happen often, so I would go with the simplest case for now.

It happens quite often that we see "random" build failures due to network errors, concurrency etc.

While in theory we should try to tackle the root of the problem, a intermediate solution would be to allow the bot to restart these builds.

In particular:

• Travis (e.g. dmd, DUB, Vibe.d) -> lots of network errors when installing DMD and sometimes even Travis errors with non-starting jobs
• Project-Tester -> lots of random errors due to the DUB registry

After making a PR, the email I get from GitHub with @dlang-bot's automatic comment looks like this:

I suggest using Markdown instead of HTML unless absolutely necessary, so that the email appears more readable in email clients configured or limited to displaying text/plain parts over text/html.

Github's squash and merge button suggests all of the commit titles, but strips merge requests. Not sure about [squash] and !fixup commit messages, but would be sensible to strip those as well.

Travis complains with the newest beta.

/home/travis/dlang/dmd-2.073.0-b1/linux/bin64/../../src/phobos/std/algorithm/iteration.d(2445,17): Error: cannot modify struct copy._items MapResult!(matchToRefs, RegexMatch!(string, BacktrackingMatcher)) with immutable members
/home/travis/dlang/dmd-2.073.0-b1/linux/bin64/../../src/phobos/std/algorithm/iteration.d(2442,28): Error: function std.algorithm.iteration.joiner!(MapResult!(matchToRefs, RegexMatch!(string, BacktrackingMatcher))).joiner.Result.save no return exp; or assert(0); at end of function
source/dlangbot/bugzilla.d(29,53): Error: template instance std.algorithm.iteration.joiner!(MapResult!(matchToRefs, RegexMatch!(string, BacktrackingMatcher))) error instantiating
/home/travis/dlang/dmd-2.073.0-b1/linux/bin64/../../src/phobos/std/algorithm/iteration.d-mixin-2437(2449,17): Error: cannot modify struct this._current Result with immutable members
/home/travis/dlang/dmd-2.073.0-b1/linux/bin64/../../src/phobos/std/algorithm/iteration.d(2446,17): Error: cannot modify struct copy._current Result with immutable members
/home/travis/dlang/dmd-2.073.0-b1/linux/bin64/../../src/phobos/std/algorithm/iteration.d(2442,28): Error: function std.algorithm.iteration.joiner!(MapResult!(__lambda2, Json[])).joiner.Result.save no return exp; or assert(0); at end of function
source/dlangbot/bugzilla.d(43,9): Error: template instance std.algorithm.iteration.joiner!(MapResult!(__lambda2, Json[])) error instantiating
dmd failed with exit code 1.

e.g. dlang/dub#1164

Idea from @CyberShadow:

Actually, would be cool if [DLang-Bot] could auto-squash --fixup commits.