bot incorrectly adding 'dont-land-on-v7.x' #116

mscdex · 2017-01-24T18:13:39Z

The bot continually adds 'dont-land-on-v7.x' even though the PR (should) lands cleanly. This has been going on for awhile now and I've had to manually remove the label in those cases.

phillipj · 2017-01-24T20:25:06Z

Got any example PRs by any chance? I could query the logs if it's a quite recent PR.

Noticed you removed it from nodejs/node#6601, but that seems correct cause those changes didn't land cleanly on the v7.x-staging branch when a tried on my machine a week or two ago.

mscdex · 2017-01-24T22:24:04Z

For example: nodejs/node#10980

or any of the recently merged test PRs (e.g. nodejs/node#10923) which should have no problem.

MylesBorins · 2017-01-24T22:31:37Z

tbqh I think we should consider dropping that auto label. incorrectly labeling don't land is potentially a big problem.

…

On Tue, Jan 24, 2017, 2:24 PM Brian White ***@***.***> wrote: For example: nodejs/node#10980 <nodejs/node#10980> or any of the recently merged test PRs (e.g. nodejs/node#10923 <nodejs/node#10923>) which should have no problem. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#116 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV9ZF_bqvkI0sMdG6r4m9DYRCznEiks5rVnoEgaJpZM4LsmbV> .

mscdex · 2017-01-24T22:36:05Z

/cc @Fishrock123

mscdex · 2017-01-25T06:50:10Z

Another example is this one: nodejs/node#10927

This one does actually have conflicts, but not any that were not automatically resolvable (no conflicting hunks). I'm thinking the bot maybe isn't set up correctly to do these safe automatic conflict resolutions?

phillipj · 2017-01-25T07:08:38Z

Logs related to ^ (10927):

06:42:51.985Z  INFO bot: Emitting GitHub event (req_id=2dabd94d-92c5-49a7-86a4-152ac4bfdc26, pr=nodejs/node/#10927, action=pull_request.synchronize)
06:42:51.986Z DEBUG bot: processing a new backport to v7 (req_id=2dabd94d-92c5-49a7-86a4-152ac4bfdc26, pr=nodejs/node/#10927, action=pull_request.synchronize)
06:42:51.986Z DEBUG bot: aborting any previous backport attempt... (req_id=2dabd94d-92c5-49a7-86a4-152ac4bfdc26, pr=nodejs/node/#10927, action=pull_request.synchronize)
06:42:52.281Z DEBUG bot: updating git remotes... (req_id=2dabd94d-92c5-49a7-86a4-152ac4bfdc26, pr=nodejs/node/#10927, action=pull_request.synchronize)
06:42:52.294Z DEBUG bot: (req_id=2dabd94d-92c5-49a7-86a4-152ac4bfdc26, pr=nodejs/node/#10927, action=pull_request.synchronize)
    Fetching origin
    
06:42:52.764Z DEBUG bot: checking out origin/v7.x-staging... (req_id=2dabd94d-92c5-49a7-86a4-152ac4bfdc26, pr=nodejs/node/#10927, action=pull_request.synchronize)
06:42:54.098Z DEBUG bot: (req_id=2dabd94d-92c5-49a7-86a4-152ac4bfdc26, pr=nodejs/node/#10927, action=pull_request.synchronize)
    Previous HEAD position was f5c57c73c5... test: cleanup stream tests
    
06:42:54.099Z DEBUG bot: (req_id=2dabd94d-92c5-49a7-86a4-152ac4bfdc26, pr=nodejs/node/#10927, action=pull_request.synchronize)
    HEAD is now at aa6b9f979e... deps: cherry-pick baba152 from V8 upstream
    
06:42:54.155Z DEBUG bot: fetching diff from pr 10927... (req_id=2dabd94d-92c5-49a7-86a4-152ac4bfdc26, pr=nodejs/node/#10927, action=pull_request.synchronize)
06:42:54.156Z DEBUG bot: attempting a backport to v7... (req_id=2dabd94d-92c5-49a7-86a4-152ac4bfdc26, pr=nodejs/node/#10927, action=pull_request.synchronize)
06:42:54.527Z DEBUG bot: (req_id=2dabd94d-92c5-49a7-86a4-152ac4bfdc26, pr=nodejs/node/#10927, action=pull_request.synchronize)
    error: patch failed: lib/buffer.js:2
    
06:42:54.527Z DEBUG bot: (req_id=2dabd94d-92c5-49a7-86a4-152ac4bfdc26, pr=nodejs/node/#10927, action=pull_request.synchronize)
    error: lib/buffer.js: patch does not apply
    
06:42:54.527Z DEBUG bot: (req_id=2dabd94d-92c5-49a7-86a4-152ac4bfdc26, pr=nodejs/node/#10927, action=pull_request.synchronize)
    Applying: buffer: improve compare() performance
    Patch failed at 0001 buffer: improve compare() performance
    The copy of the patch that failed is found in: .git/rebase-apply/patch
    When you have resolved this problem, run "git am --continue".
    If you prefer to skip this patch, run "git am --skip" instead.
    To restore the original branch and stop patching, run "git am --abort".
    
06:42:54.528Z DEBUG bot: backport to 7 failed (req_id=2dabd94d-92c5-49a7-86a4-152ac4bfdc26, pr=nodejs/node/#10927, action=pull_request.synchronize)
06:42:54.529Z DEBUG bot: Resolved labels: dont-land-on-v7.x (req_id=2dabd94d-92c5-49a7-86a4-152ac4bfdc26, pr=nodejs/node/#10927, action=pull_request.synchronize)

phillipj · 2017-01-25T07:29:21Z

Logs related to 10980 looks weird:

17:57:40.092Z DEBUG bot: processing a new backport to v7 (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:40.093Z DEBUG bot: aborting any previous backport attempt... (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:40.417Z DEBUG bot: updating git remotes... (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:40.432Z DEBUG bot: (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
    Fetching origin
    
17:57:40.919Z DEBUG bot: checking out origin/v7.x-staging... (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:42.100Z DEBUG bot: Fetching PR files for labelling (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:42.287Z DEBUG bot: (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
    Previous HEAD position was f5c57c73c5... test: cleanup stream tests
    
17:57:42.288Z DEBUG bot: (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
    HEAD is now at aa6b9f979e... deps: cherry-pick baba152 from V8 upstream
    
17:57:42.350Z DEBUG bot: fetching diff from pr 10980... (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:42.351Z DEBUG bot: attempting a backport to v7... (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:43.149Z DEBUG bot: (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
    .git/rebase-apply/patch:6498: trailing whitespace.
    ;    IPos cur_match);                             // current match 
    
17:57:43.150Z DEBUG bot: (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
    .git/rebase-apply/patch:6794: trailing whitespace.
    
    
17:57:43.150Z DEBUG bot: (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
    .git/rebase-apply/patch:6795: trailing whitespace.
    
    
17:57:43.150Z DEBUG bot: (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
    .git/rebase-apply/patch:6796: trailing whitespace.
    
    
17:57:43.150Z DEBUG bot: (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
    .git/rebase-apply/patch:6803: trailing whitespace.
    
    
17:57:43.162Z DEBUG bot: e (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:43.162Z DEBUG bot: rror:  (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:43.162Z DEBUG bot: p (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:43.162Z DEBUG bot: atch failed:  (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:43.162Z DEBUG bot: d (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:43.162Z DEBUG bot: eps/zlib/contrib/minizip/ChangeLogUnzip (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:43.162Z DEBUG bot: : (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:43.163Z DEBUG bot: 1 (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:43.163Z DEBUG bot: (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
    
    
17:57:43.163Z DEBUG bot: e (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:43.163Z DEBUG bot: rror:  (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:43.163Z DEBUG bot: d (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:43.163Z DEBUG bot: eps/zlib/contrib/minizip/ChangeLogUnzip (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:43.163Z DEBUG bot: : (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:43.163Z DEBUG bot:  patch does not apply (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:43.164Z DEBUG bot: (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
    
    
17:57:43.178Z DEBUG bot: (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
    Applying: deps: upgrade zlib to 1.2.11
    Patch failed at 0001 deps: upgrade zlib to 1.2.11
    The copy of the patch that failed is found in: .git/rebase-apply/patch
    When you have resolved this problem, run "git am --continue".
    If you prefer to skip this patch, run "git am --skip" instead.
    To restore the original branch and stop patching, run "git am --abort".
    
17:57:43.180Z DEBUG bot: backport to 7 failed (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)
17:57:43.180Z DEBUG bot: Resolved labels: dont-land-on-v7.x (req_id=be04f999-798d-43f7-bad2-ae01a13b2820, pr=nodejs/node/#10980, action=pull_request.opened)

@MylesBorins said:
tbqh I think we should consider dropping that auto label. incorrectly
labeling don't land is potentially a big problem.

Totally understandable. It's trivial to disable the actual labelling, tho still perform the backport attempts for debugging what causes these issues.. I'd let @Fishrock123 decide when/what to do.

@mscdex would you say any backporting attempt labels are buggy, or mainly dont-land-on-v7.x?

mscdex · 2017-01-25T07:29:26Z

@phillipj Right, but there are no conflicting hunks, so an automatic resolution should be possible. For example, for that particular PR I merged with git am -3 and when I then ran git mergetool, meld showed no conflicting hunks so all I had to do was save the file and --continue.

mscdex · 2017-01-25T07:32:54Z

@phillipj I don't know about the other labels, but definitely dont-land-on-v7.x. I would think though since the process is the same for all branches, it's probably the same issue for all the labels.

phillipj · 2017-01-25T07:53:24Z

@mscdex ahh good catch, looking at ./scripts/attempt-backport.js#L217 it seems we're not passing the -3 arg.

sam-github · 2017-01-25T17:52:14Z

I agree with @MylesBorins about #116 (comment)

We should distinguish the auto-labels and the human labels (assuming the auto labels can actually be fixed).

The auto-label should be something like does-not-land, or auto-merge-to-v7.x-failed, so it clearly describes what the issue is, and the PR submitter can try to fix.

An opinion on dont-land should be reserved for humans to make.

sam-github · 2017-01-25T18:21:58Z

It gets re-added incorrectly to nodejs/node#10980 on every rebase, so if you are looking for a repro, its easy.

mscdex · 2017-01-25T18:54:25Z

I've always been a proponent of having separate labels for intention vs. technically landable without human intervention.

phillipj · 2017-01-25T20:25:52Z

Interesting discussion, personally not too opinionated as long as the devs using these labels to filter/exclude commits to land on staging branches is happy.

Fixing auto labels is needed anyway. I opened #117 enabling 3-way merge which will hopefully increase the number of PRs landing cleanly, and thereby not adding invalid auto labels.

mscdex · 2017-01-25T20:29:24Z

AFAIK those that update the staging branches every so often already check if commits don't land cleanly, so it seems like they don't rely on the labels anyway. I believe the labels were more there for the PR author's benefit?

sam-github · 2017-01-25T20:35:31Z

The tools to update the staging branch need a label to use when a PR is known to be irrelevant for, so they can skip it, that label is dont-land.

The bot's labelling is probably useful for the author's benefit, but unfortunately, the bot is currently applying the dont-land label, automatically, which is a misuse of the label, and causing confusion.

gibfahn · 2017-01-26T23:19:19Z

@MylesBorins

tbqh I think we should consider dropping that auto label. incorrectly
labeling don't land is potentially a big problem.

@sam-github

The bot's labelling is probably useful for the author's benefit, but unfortunately, the bot is currently applying the dont-land label, automatically, which is a misuse of the label, and causing confusion.

@mscdex (#90 (comment))

I think having the bot applying the dont-land-on-* labels is a bit of a misnomer. My understanding of those labels is that they indicate intention, not whether the PR can currently be backported cleanly or not. I think the recent PR that @Fishrock123 pointed out is a good example of this problem.
I think a different set of labels should be used by the bot, but the problem with that is that we're already hitting the 100 label ceiling (this is a separate issue that needs discussion too, because not everyone is aware of it).

+1, the dont-land-on-vX.x label means it shouldn't land at all, not that it doesn't land cleanly. If we want to have a label to say that something doesn't land cleanly on a branch, we should use a different one. Otherwise we'll miss PRs that should have been backported but were missed because they didn't backport cleanly and thus got a dont-land-on-v7.x label.

Can we stop the bot applying the dont-land-on-v7.x label? I think it's being done in scripts/attempt-backport.js#L134-L135, which was added in #90.

cc/ @Fishrock123 as I don't really know anything about the Github bot 😅

I'd also note that the 100 labels issue has now been fixed, so if we want to add bot labels to help with releases that should be straightforward (we should document them somewhere though).

There's been some concerns about auto labelling `dont-land-on-*` labels lately, especially since the bot is often wrongly adding these labels ATM. Therefore temporarily disabling that functionality to avoid further damage. Refs nodejs#116

There's been some concerns about auto labelling `dont-land-on-*` labels lately, especially since the bot is often wrongly adding these labels ATM. Therefore temporarily disabling that functionality to avoid further damage. Refs #116

phillipj · 2017-01-27T09:31:24Z

The auto labelling of dont-land-on-* has been disabled for now.

phillipj · 2017-01-29T20:09:49Z

Closing this since dont-land-on-* has been temp disabled, and moving the unanswered discussion into #120.

phillipj mentioned this issue Jan 25, 2017

attempt-backport: enable 3-way merge for git am #117

Merged

phillipj mentioned this issue Jan 27, 2017

attempt-backport: temporary disable dont-land-on-* labels #118

Merged

sam-github mentioned this issue Jan 27, 2017

Proposal to update v7.x-staging nodejs/node#10989

Closed

phillipj mentioned this issue Jan 29, 2017

Which backport PR labels should be added by the bot? #120

Open

phillipj closed this as completed Jan 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bot incorrectly adding 'dont-land-on-v7.x' #116

bot incorrectly adding 'dont-land-on-v7.x' #116

mscdex commented Jan 24, 2017

phillipj commented Jan 24, 2017

mscdex commented Jan 24, 2017

MylesBorins commented Jan 24, 2017 via email

mscdex commented Jan 24, 2017

mscdex commented Jan 25, 2017

phillipj commented Jan 25, 2017

phillipj commented Jan 25, 2017

mscdex commented Jan 25, 2017

mscdex commented Jan 25, 2017 •

edited

Loading

phillipj commented Jan 25, 2017

sam-github commented Jan 25, 2017

sam-github commented Jan 25, 2017

mscdex commented Jan 25, 2017

phillipj commented Jan 25, 2017

mscdex commented Jan 25, 2017

sam-github commented Jan 25, 2017

gibfahn commented Jan 26, 2017

phillipj commented Jan 27, 2017

phillipj commented Jan 29, 2017

bot incorrectly adding 'dont-land-on-v7.x' #116

bot incorrectly adding 'dont-land-on-v7.x' #116

Comments

mscdex commented Jan 24, 2017

phillipj commented Jan 24, 2017

mscdex commented Jan 24, 2017

MylesBorins commented Jan 24, 2017 via email

mscdex commented Jan 24, 2017

mscdex commented Jan 25, 2017

phillipj commented Jan 25, 2017

phillipj commented Jan 25, 2017

mscdex commented Jan 25, 2017

mscdex commented Jan 25, 2017 • edited Loading

phillipj commented Jan 25, 2017

sam-github commented Jan 25, 2017

sam-github commented Jan 25, 2017

mscdex commented Jan 25, 2017

phillipj commented Jan 25, 2017

mscdex commented Jan 25, 2017

sam-github commented Jan 25, 2017

gibfahn commented Jan 26, 2017

phillipj commented Jan 27, 2017

phillipj commented Jan 29, 2017

mscdex commented Jan 25, 2017 •

edited

Loading