auto-merge should obey combined CI status by MartinNowak · Pull Request #69 · dlang/dlang-bot

MartinNowak · 2018-01-16T03:40:37Z

Not only status from required tests. If some tests fail ppl. should merge manually.

The main motivation here is to avoid accidentally merging PRs w/ CI issues that weren't yet visible when adding the auto-merge label.

andralex · 2017-04-05T16:06:10Z

Thanks!

andralex · 2017-04-05T16:09:21Z

Ah, that's just an issue not the fix :)

CyberShadow · 2017-04-05T16:27:17Z

@MartinNowak Can you make it so when someone adds the auto-merge label on an issue @dlang-bot submits a pull request with a fix? 🤣

MartinNowak · 2017-06-29T11:22:22Z

rm -rf code

wilzbach · 2017-07-15T22:51:55Z

How would we notify contributors that a PR is having the "auto-merge" label, but isn't merged due to issues with CI X. Plain-old email?
Also we definitely would want to exclude CodeCov from this list.

CyberShadow · 2017-07-15T22:52:43Z

How would we notify contributors that a PR is having the "auto-merge" label, but isn't merged due to issues with CI X. Plain-old email?

Just update the comment?

wilzbach · 2017-07-15T22:53:40Z

Just update the comment?

And how would you notice this?

CyberShadow · 2017-07-15T22:57:43Z

I think the CI status being red should be enough of an indicator on its own, with the explanation in the comment being supplementary. Right now it's just that people are used that auto-merge works even with failing CI services, but breaking old expectations would be a matter of time.

Maybe I don't understand the problem.

MartinNowak · 2018-01-15T08:47:15Z

Also we definitely would want to exclude CodeCov from this list.

No we don't, red is red, there are no two ways about.
We fix any broken CIs, green is green.
We use required only for CIs that definitely need admin feedback to be overridden (cov and lint no, tests and broken code/projectd yes).
The bot never overrides any CI or tries to merge with pending CIs. Auto-merge will be untagged when a CI fails.

wilzbach · 2018-01-15T23:04:37Z

Auto-merge will be untagged when a CI fails.

I wouldn't do that. I often have to restart your spurious Jenkins instance. It would be annoying to re-add auto-merge on every restart (even if the approved PR code didn't change).
Also the label "auto-merge" makes them easy to find (e.g. by label or on auto-tester), so it increases their exposure and the chances that someone looks at the CI failure.

MartinNowak · 2018-01-15T23:14:12Z

OK granted for now that would be counter-productive.

- for more code consistency/symmetry - also fix orphan %s/%s format specifier

- do not auto-merge when one of the CIs fails or still is pending - fixed by querying combined status before getting more infos and trying to merge - this means dlang-bot will no longer treat CIs as lesser cousins of required CIs

dlang-bot · 2018-01-16T03:40:38Z

Thanks for your pull request, @MartinNowak! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

smaller, focused PRs are easier to review than big ones
try not to mix up refactoring or style changes with bug fixes or feature enhancements
provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

MartinNowak · 2018-01-16T03:41:43Z

Now implemented and converted the issue to a pull request.

wilzbach

Can we please withhold this until we have fixed Travis for DMD?
No one wants to wait >24h for it to pass or more likely time out. All other CIs are enforced at the DMD repos anyways.

wilzbach · 2018-01-16T03:49:02Z

source/dlangbot/cron.d

-        if (e.state == GHCiStatus.State.failure ||
-            e.state == GHCiStatus.State.error)
+    // TODO: unclear whether we want all statuses for PR commit, or only the latest
+    auto status = t.pr.combinedStatus;


This will be yet another request for the cron job. Each request multiplies with ~400 here.

Only open auto-merge PRs, not so many.

wilzbach · 2018-01-16T03:50:33Z

source/dlangbot/github_api.d

+
+        return ghGetRequest(_pullRequest.url)
+                .readJson
+                .deserializeJson!PullRequest;


This will silently lead to a lot more requests.

Hard to avoid, issues have no head commit, but on the plus side we're saving many requests because merge attempts for non-green PRs can be canceled early on.

wilzbach · 2018-01-16T03:57:42Z

source/dlangbot/cron.d

-            e.state == GHCiStatus.State.error)
+    // TODO: unclear whether we want all statuses for PR commit, or only the latest
+    auto status = t.pr.combinedStatus;
+    auto failCount = status.latestStatuses.filter!((e){


FWIW both are the same:

https://api.github.com/repos/dlang/phobos/commits/a04acd6a2813fb344d3e47369cf7fd64523ece44/status
https://api.github.com/repos/dlang/phobos/status/a04acd6a2813fb344d3e47369cf7fd64523ece44

Yes, I just went with the one from the API docs.

wilzbach · 2018-01-16T04:05:58Z

Also - I'm not sure whether you are aware - but we use the dlang-bot at dlang-community where Windows is still often failing:

dlang-community/D-Scanner#546

It's not a huge deal as we can simply remove the failing Windows builds.

wilzbach · 2018-01-16T04:07:47Z

source/dlangbot/github.d

            continue;

-        auto pr = issue.toPullRequest;
+        auto pr = issue.pullRequest;


The motivation behind toPullRequest was to save API requests.

Yes, I know, but not avoidable if we want to check the status. Issues don't have commits.
Those are only open PRs with auto-merge btw, a rather small number.

open auto-merges:
dmd -> 1
druntime -> 1
phobos -> 1
dlang.org -> 1
installer -> 0
tools -> 0

And that's only because of the merge_stable step.

wilzbach · 2018-01-16T04:27:41Z

X codecov/patch - 89.361% of diff hit (target 91.08%)

Kind of ironic that dlang-bot wouldn't merge this PR?
The trick is informational: true -> https://github.com/dlang/dmd/blob/master/.codecov.yml

MartinNowak · 2018-01-16T04:43:36Z

Sure it's going to be some hassle, but I'm positive that it will help us to clean our CI forest.
Also we can now use required to really mean only an admin can make this decision, rather than please consider this check.
And in the end we could still revert this if it doesn't work out.

BTW, -0.649% coverage drop indeed ask for an explanation and shouldn't be silently skipped. Could we just increase the tolerance below the target for codecov/patch a bit.

Without required non-admins can decide to skip coverage ;).

MartinNowak · 2018-01-16T04:47:19Z

Here comes the coverage bump, dropping untested and unused code.

wilzbach · 2018-01-16T04:51:42Z

Sure it's going to be some hassle, but I'm positive that it will help us to clean our CI forest.

Don't get me wrong. I'm fully in favor of improving the CIs and I have been trying to improve the reliability of them for the last weeks (still waiting for your approval on dlang/installer#291 BTW).
I'm just pointing out that if we go through with this, we have to kill Travis on DMD because currently dlang-bot always merged with 9/10 status checks passing and 1 pending. See dlang/dmd#7617 (comment) for details.

MartinNowak · 2018-01-16T05:00:07Z

That backlog will be gone at some point, no?
Travis CI Status - Reduced macOS capacity
And even if not, what's the worth of a CI that doesn't report back in 1 day.
IIRC the OSX test was only added for 32-bit anyhow, right?

wilzbach · 2018-01-16T05:16:29Z

That backlog will be gone at some point, no?

I'm not so sure, we could try to disable builds on master to get more throughput, but that won't prevent cases where we run over the maximal build time which is still very frequent.
I have been actively cancelling builds, but the history still doesn't look good:

https://travis-ci.org/dlang/dmd/builds

IIRC the OSX test was only added for 32-bit anyhow, right?

Ehm didn't you add the OSX test?

dlang/dmd@b2c10f8

And even if not, what's the worth of a CI that doesn't report back in 1 day.

Yes my point!
If I understand correctly the point of TravisCI was to test whether we can bootstrap master with {dmd,ldc,gdc} and SemaphoreCI does this for the time being pretty reliable.
I wasn't sure how much value testing the bootstrapping with the latest DMD on OSX brings as we already build the latest DMD on auto-tester on OSX (though auto-tester does no rebuild against the newly built compiler).

wilzbach · 2018-01-16T05:28:39Z

And even if not, what's the worth of a CI that doesn't report back in 1 day.

-> dlang/dmd#7721

MartinNowak · 2018-01-16T19:09:28Z

Ehm didn't you add the OSX test?

dlang/dmd@b2c10f8

Yes, but I vaguely remember why. Looks like it was for the missing clang coverage on the auto-tester at that time. Since Brad updated the toolchain/OSXes it's no longer necessary dlang/druntime#1542

wilzbach · 2018-01-16T19:59:51Z

Travis is gone from DMD -> we can merge this now :)

wilzbach mentioned this pull request Mar 28, 2017

fix -c -inline ddmd/mars.d dlang/dmd#6657

Merged

This was referenced Apr 3, 2017

Fix Issue 15896 - private ignored when import bindings are used dlang/dmd#6660

Merged

Make allocators' resolveInternalPointer take a const pointer as source dlang/phobos#5321

Merged

andralex added the auto-merge label Apr 5, 2017

MartinNowak mentioned this pull request Jun 30, 2017

Failed to Lock when fetching lib dlang/dub#1149

Merged

MartinNowak mentioned this pull request Jul 14, 2017

Fix style (space between a .. b) in std/json.d dlang/phobos#5560

Merged

MartinNowak removed the auto-merge label Jul 14, 2017

wilzbach mentioned this pull request Jul 15, 2017

Replace idgen with CTFE implementation dlang/dmd#6837

Merged

CyberShadow mentioned this pull request Jul 17, 2017

Fix issue 17392 - Add Dub file for the lexer and parser dlang/dmd#6771

Merged

MartinNowak mentioned this pull request Jan 15, 2018

Reapply PR #209, which got accidentally reverted in #203 previously. dlang/dub-registry#262

Merged

MartinNowak added 5-sol 4-fa bug and removed 5-sol labels Jan 15, 2018

MartinNowak mentioned this pull request Jan 15, 2018

Add merge_stable action to dlang-bot #165

Open

factor out handleReview into a separate method

063acc3

- for more code consistency/symmetry - also fix orphan %s/%s format specifier

MartinNowak changed the title ~~auto-merge should obey integrated CI status~~ auto-merge should obey combined CI status Jan 16, 2018

dlang-bot added the Bug Fix label Jan 16, 2018

MartinNowak requested a review from wilzbach January 16, 2018 03:40

wilzbach reviewed Jan 16, 2018

View reviewed changes

remove unused and untested statuses api

20a0cae

wilzbach mentioned this pull request Jan 16, 2018

Remove Travis testing dlang/dmd#7721

Merged

wilzbach merged commit 41104e1 into dlang:master Jan 16, 2018

MartinNowak deleted the fix69 branch January 16, 2018 20:17

This was referenced Jan 17, 2018

Allow to run the DMD testsuite on hardened systems dlang/dmd#7420

Merged

Always set CodeCov notifications to green/passing dlang/dub#1347

Merged

CyberShadow mentioned this pull request May 27, 2021

Don't merge PRs where non-required status are failing #243

Closed

Uh oh!

Comments

Conversation

MartinNowak commented Jan 16, 2018

Uh oh!

andralex commented Apr 5, 2017

Uh oh!

andralex commented Apr 5, 2017

Uh oh!

CyberShadow commented Apr 5, 2017

Uh oh!

MartinNowak commented Jun 29, 2017

Uh oh!

wilzbach commented Jul 15, 2017

Uh oh!

CyberShadow commented Jul 15, 2017

Uh oh!

wilzbach commented Jul 15, 2017

Uh oh!

CyberShadow commented Jul 15, 2017

Uh oh!

MartinNowak commented Jan 15, 2018

Uh oh!

wilzbach commented Jan 15, 2018

Uh oh!

MartinNowak commented Jan 15, 2018

Uh oh!

dlang-bot commented Jan 16, 2018

Uh oh!

MartinNowak commented Jan 16, 2018

Uh oh!

wilzbach left a comment

Choose a reason for hiding this comment

Uh oh!

wilzbach Jan 16, 2018

Choose a reason for hiding this comment

Uh oh!

MartinNowak Jan 16, 2018

Choose a reason for hiding this comment

Uh oh!

wilzbach Jan 16, 2018

Choose a reason for hiding this comment

Uh oh!

MartinNowak Jan 16, 2018

Choose a reason for hiding this comment

Uh oh!

wilzbach Jan 16, 2018

Choose a reason for hiding this comment

Uh oh!

MartinNowak Jan 16, 2018

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jan 16, 2018

Uh oh!

wilzbach Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MartinNowak Jan 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wilzbach commented Jan 16, 2018

Uh oh!

MartinNowak commented Jan 16, 2018

Uh oh!

MartinNowak commented Jan 16, 2018

Uh oh!

wilzbach commented Jan 16, 2018

Uh oh!

MartinNowak commented Jan 16, 2018

Uh oh!

wilzbach commented Jan 16, 2018

Uh oh!

wilzbach commented Jan 16, 2018

Uh oh!

MartinNowak commented Jan 16, 2018

Uh oh!

wilzbach commented Jan 16, 2018

Uh oh!

wilzbach Jan 16, 2018 •

edited

Loading

MartinNowak Jan 16, 2018 •

edited

Loading