Forks on repo list show as permanently building

[gtaylor]

Sometime between July 20 and July 25th, a commit landed that caused our forks to display as permanently building on the repo list. The left side's build feed is still correctly going through the building->success/fail transition.

I'm not able to get into git blame'ing this right now, but hopefully this is reproducible beyond our setup.

[Tathanen]

If you've been pulling down versions of this before the 3.1.0 release, it's possible you've got some crummy data stored in localstorage that needs to be flushed out. The local settings will override any that are set during the build, and there may have been something that needs to be changed but is being overridden by old bung data. Try deleting the settings value in local storage and let me know if you continue to see the issue.

[gtaylor]

The whole UI was showing "Waiting for builds" before this, so I did end up clearing local storage. This issue was filed after that was done. In theory, my localstorage should have been empty.

[Tathanen]

Could you share the contents of your localstorage settings value, a screenshot of the wall, and maybe a sample API response? Obfuscated where appropriate.

Maybe just start with the screenshot, to make sure we're on the same page regarding the behavior. There are two pretty different things this could be based on what it actually looks like.

[gtaylor]

Sorry for the delay. Since we had to restart the dash machine today anyway, I completely nuked localstorage again. Here's a screenshot:

I can't show the repo names right now, but "Latest Builds" has nothing building. Confirmed in Drone that no builds are happening. These PRs stay stuck in the building state indefinitely. All of our repos with PRs have spinny blue state.

Another important detail is that we are cheating and running this under npm start like so:

npm start -- -env=dev -theme=dark -orgname=Reddit -apiroot=https://cihostname/api/ -token=token

We then point the dash computer's browser at localhost and serve directly from there. I don't know if that would complicate things.

[Tathanen]

...huh. So the builds complete and transition to success/failure in the latest builds section, just not in the pull-request tabs? Do the PR tabs disappear when they're merged, or do you just accrue them indefinitely until the 48 hour timeout clears them?

I'm not seeing this on our end at all, so it's quite curious. I'd say you aren't getting "finished" status updates from drone for some reason, but if builds complete on the left that can't be it.

I'd like to rule out that your drone feed is in some way aberrant. There's a mock feed response in the repo and the tests progress a build from in-progress to complete successfully, could you compare your feed response to it? Ideally a pre- and post-build-completion comparison?

[gtaylor]

...huh. So the builds complete and transition to success/failure in the latest builds section, just not in the pull-request tabs?

Correct.

Do the PR tabs disappear when they're merged, or do you just accrue them indefinitely until the 48 hour timeout clears them?

It's honestly pretty tough to tell. We see tons of PR builds each week. It seems like they stick around indefinitely, but I'm not 100% positive.

I'll try the comparison on our next 20% day.

[Tathanen]

So a theoretical failure point may be here:

if( build.event === "pull_request" )
{
    match = build.ref.match( /refs\/pull\/([0-9]+)\/merge/i );
    return match ? match[ 1 ] : null;
}
else if( build.event === "push" )
{
    match = build.message.match( /Merge pull request #([0-9]+)/i );
    return match ? match[ 1 ] : null;
}

I parse the build ref and message parameters to find the pull request ID, and use that ID to find the PR tab in the repo list that's associated with a new build status, and update it. It's how that area is distinct from the "latest builds" list, which updates build statuses based upon the commit hash.

This works in my version, and certainly used to work in your version. But if for some reason your messages are in a different format now, a new status reporting on the finishing of a PR build would be disassociated from the existing PR tab.

SO, that's the number one area for you check. See if the messages on your merge pushes don't match that regex.

[gtaylor]

Ahhhh, I bet I know what's going on. We're not using that /merge/ ref anymore, since merge conflicts were resulting in builds failing to clone.

On Sep 2, 2016 6:41 PM, "Cory Faller" notifications@github.com wrote:

So a theoretical failure point may be here:

if( build.event === "pull_request" ) { match = build.ref.match( /refs\/pull\/([0-9]+)\/merge/i ); return match ? match[ 1 ] : null; } else if( build.event === "push" ) { match = build.message.match( /Merge pull request #([0-9]+)/i ); return match ? match[ 1 ] : null; }

I parse the build ref and message parameters to find the pull request ID, and use that ID to find the PR tab in the repo list that's associated with a new build status, and update it. It's how that area is distinct from the "latest builds" list, which updates build statuses based upon the commit hash.

This works in my version, and certainly used to work in your version. But if for some reason your messages are in a different format now, a new status reporting on the finishing of a PR build would be disassociated from the existing PR tab.

SO, that's the number one area for you check. See if the messages on your merge pushes don't match that regex.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/drone/drone-wall/issues/45#issuecomment-244519555, or mute the thread https://github.com/notifications/unsubscribe-auth/AAEnJPIHpw3eAXWAlAhLfQb4w9TCKZnNks5qmNBJgaJpZM4JUcs2 .

[Tathanen]

Aha! That'd probably do it.

Unfortunately there's no other place in a pull_request type event that contains the PR ID. It's not great that I have to scrape the ref and message fields anyway, particularly the message field which I can see changing due to any number of reasons in the future. I'll open an issue on the main drone repo to see if it'd be possible to include the PR ID as its own dedicated field on the feed route, which would resolve this issue here pretty cleanly.

Unless that works out tho, restoring the ref field is likely your only real path to success.

[gtaylor]

Unless that works out tho, restoring the ref field is likely your only real path to success.

As in going back to the auto-merge ref? We definitely can't do that at this point. It was causing some serious confusion. It gives the appearance of builds failing randomly, even though there's a very logical reason (merge conflicts).

Might it be worth disabling the PR state spinner entirely for those with this build ref override set? Also, it sounds like the non-merge ref may be made the default in 0.5 or later.