build: require "allow edits" to be checked #35002

ljharb · 2020-08-31T22:37:03Z

This github action will fail if "allow edits" is not checked - this allows all PRs to be landed purple.

Open to any feedback for tweaking error messages, and if there's concern with depending on main, i'm happy to use tags, or make an explicit node branch, so node's usage of the action can be more precisely targeted.

See also, #35001

mmarchini · 2020-08-31T22:38:56Z

.github/workflows/require-allow-edits.yml

+on: [pull_request_target]
+
+jobs:
+  _:


The display on the PR will be Require "Allow Edits" / _ with this. If there's a different name "after the slash" you'd prefer, I'm happy to change it.

ohh hmm, looks like by providing the name field the _ is overridden :-)

What display text would you like?

If the name field is used I'm fine with it. I asked because I thought _ had some implicit hidden behavior maybe 😅

.github/workflows/require-allow-edits.yml

mscdex · 2020-08-31T23:22:16Z

This will require all PRs to allow edits?

ljharb · 2020-08-31T23:24:00Z

@mscdex yes, but that's the default, and in my experience vanishingly few people turn it off.

mscdex

I don't think this option should be required for all PRs. I personally disable the option every time because I prefer being in control of what changes are made to my PRs, so that there are no surprises.

I am fine with the merge feature or whatever being opt-in for those that want to enable the "allow edits" option.

mmarchini · 2020-09-01T00:22:39Z

I don't have strong opinions, my PR #35001 already takes into account PRs with and without "allow edits" 🤷🏻‍♀️

ljharb · 2020-09-01T00:48:39Z

@mscdex to be clear; this PR doesn't actually make it required on all PRs, but I'm told that NCU doesn't differentiate between required and optional PR status checks, which makes everything effectively required.

mscdex · 2020-09-01T03:43:59Z

@mscdex to be clear; this PR doesn't actually make it required on all PRs

That was what I asked previously and you said yes, so now I'm confused.

but I'm told that NCU doesn't differentiate between required and optional PR status checks, which makes everything effectively required.

I'm assuming "NCU" is referring to node-core-utils here? If so, I don't quite follow.

All I care about is being able to create PRs with "allow edits" disabled. I'm not using node-core-utils or anything fancy like that, just plain old git commands to do everything. As long as that kind of workflow won't be affected, then I will be happy to withdraw my -1.

ljharb · 2020-09-01T04:02:01Z

@mmarchini can you confirm?

My assumption is that as long as you’re landing your own PR, this requirement won’t affect you; it’s for when someone else needs to land the PR.

mmarchini · 2020-09-01T04:14:37Z

Nope, the bot has a different token (it doesn't pretend to be you), so it won't be able to force push to head branch (it won't be able to purple merge if "allow edit" is not checked)

mmarchini · 2020-09-01T04:17:44Z

Also for some context @ljharb, we don't have "required" checks (the GitHub setting) in this repository.

ljharb · 2020-09-01T04:51:22Z

@mmarchini ok, but if someone lands the PR without using the bot, will it still work?

ljharb · 2020-09-01T04:52:01Z

@mscdex it's also worth noting that you could, at the final moment before merging, check the box and rerun the one workflow, and then land it even with ncu?

mmarchini · 2020-09-01T04:55:10Z

Yes, ncu let's you bypass the check if necessary. And I might be misinterpreting, but I think @mscdex doesn't use node-core-utils to land PRs.

mscdex · 2020-09-01T13:28:26Z

So then the changes in this PR do not affect the UI when I click "New pull request" on the website, correct?

ljharb · 2020-09-01T14:57:25Z

Correct; that’s not something a repo can control beyond the pull request template - this PR is just an action for opened PRs.

PR-URL: nodejs#35002 Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Trott · 2020-09-04T03:40:43Z

Landed in 07423b5

PR-URL: #35002 Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: Rich Trott <[email protected]>

addaleax · 2020-09-07T14:32:34Z

This currently shows up as a red ❌ on pull requests when allow-edits isn’t possible, and keeps the CQ from working as it seems, e.g. #35067 – maybe this should be reverted?

PR-URL: #35002 Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: Rich Trott <[email protected]>

ljharb · 2020-09-07T16:54:08Z

@addaleax when is "allow edits" not possible? in that case, the user appears to have not checked the box.

addaleax · 2020-09-07T17:28:04Z

@ljharb It’s not possible when it comes from an organization fork rather than the author’s personal one.

More importantly, the author should absolutely not have to check that box. I read the conversation with @mscdex above as implying that everything would still work when this box is not checked, except that the PR would not get a purple merge badge. I’m sorry for misunderstanding, but I think this is reason enough to revert this for me.

ljharb · 2020-09-07T18:06:41Z

It's certainly possible, but due to a github bug, checking the box won't allow force pushing to the branch (but it will cause the PR check to pass).

Everything will indeed still work when it's an X, just like it did on the PR you referenced.

mmarchini · 2020-09-07T18:52:01Z

Everything will indeed still work when it's an X, just like it did on the PR you referenced

Except the CQ will fail, as Anna mentioned (also worth noting that this PR landed before the purple CQ merge). If this is getting in the way for folks I'm fine reverting.

richardlau · 2020-09-07T19:07:07Z

If it's stopping the CQ from landing things it was able to land before let's revert.

In terms of the CQ purple merging I think the CQ should cope with both PR's that allow edits and PR's that do not as we do not currently require PR's to allow edits (to me that feels unnecessarily restrictive).

ljharb · 2020-09-07T19:18:25Z

Ah, that's something I missed, in that case a revert does make sense.

It seems like it would be a better approach to use Github's branch protections to mark specific checks as "required", and then fix CQ and relevant tools so they only fail when required checks fail. That way, this check can land and be optional, and it wouldn't interfere?

mmarchini · 2020-09-07T19:30:32Z

It seems like it would be a better approach to use Github's branch protections to mark specific checks as "required", and then fix CQ and relevant tools so they only fail when required checks fail

It's a lot more work than it seems. We have doc-only change rules where tests don't need to run, we have Jenkins status which are posted to GH via Status API instead of Checks API (and have erratic behavior sometime), and we have enough flaky tests that an override is frequently necessary.

Either way, this needs to be first a change to our collaborators guide, and such a change will need to follow the overall consensus process as well as have someone willing to implement and fix all the pieces.

This reverts commit 07423b5. Refs: nodejs#35002 (comment) PR-URL: nodejs#35094 Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: Richard Lau <[email protected]>

This reverts commit 07423b5. Refs: #35002 (comment) PR-URL: #35094 Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: Richard Lau <[email protected]>

PR-URL: nodejs#35002 Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: Rich Trott <[email protected]>

This reverts commit 07423b5. Refs: nodejs#35002 (comment) PR-URL: nodejs#35094 Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: Richard Lau <[email protected]>

ljharb added the tools Issues and PRs related to the tools directory. label Aug 31, 2020

ljharb requested a review from mmarchini August 31, 2020 22:37

nodejs-github-bot added the meta Issues and PRs related to the general management of the project. label Aug 31, 2020

mmarchini approved these changes Aug 31, 2020

View reviewed changes

ljharb force-pushed the ljharb/require_allow_edits branch from 063cb9f to d6d01ff Compare August 31, 2020 22:45

mscdex previously requested changes Aug 31, 2020

View reviewed changes

Trott approved these changes Sep 4, 2020

View reviewed changes

build: require "allow edits" to be checked

07423b5

PR-URL: nodejs#35002 Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Trott force-pushed the ljharb/require_allow_edits branch from d6d01ff to 07423b5 Compare September 4, 2020 03:40

Trott merged commit 07423b5 into nodejs:master Sep 4, 2020

ljharb deleted the ljharb/require_allow_edits branch September 4, 2020 06:10

richardlau pushed a commit that referenced this pull request Sep 7, 2020

build: require "allow edits" to be checked

76dc189

PR-URL: #35002 Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: Rich Trott <[email protected]>

richardlau mentioned this pull request Sep 7, 2020

v14.10.0 proposal #35023

Merged

4 tasks

richardlau pushed a commit that referenced this pull request Sep 7, 2020

build: require "allow edits" to be checked

1be6956

PR-URL: #35002 Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: Rich Trott <[email protected]>

ljharb mentioned this pull request Sep 7, 2020

stream: improve Writable.destroy performance #35067

Closed

4 tasks

Trott mentioned this pull request Sep 7, 2020

Revert "build: require "allow edits" to be checked" #35094

Merged

1 task

ruyadorno pushed a commit that referenced this pull request Sep 17, 2020

Revert "build: require "allow edits" to be checked"

f3c45a1

This reverts commit 07423b5. Refs: #35002 (comment) PR-URL: #35094 Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: Richard Lau <[email protected]>

addaleax added dont-land-on-v12.x labels Sep 21, 2020

joesepi pushed a commit to joesepi/node that referenced this pull request Jan 8, 2021

build: require "allow edits" to be checked

63247c6

PR-URL: nodejs#35002 Reviewed-By: Mary Marchini <[email protected]> Reviewed-By: Rich Trott <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

build: require "allow edits" to be checked #35002

build: require "allow edits" to be checked #35002

ljharb commented Aug 31, 2020 •

edited

Loading

mmarchini Aug 31, 2020

ljharb Aug 31, 2020

ljharb Aug 31, 2020 •

edited

Loading

mmarchini Aug 31, 2020

mscdex commented Aug 31, 2020

ljharb commented Aug 31, 2020

mscdex left a comment

mmarchini commented Sep 1, 2020

ljharb commented Sep 1, 2020

mscdex commented Sep 1, 2020

ljharb commented Sep 1, 2020

mmarchini commented Sep 1, 2020 •

edited

Loading

mmarchini commented Sep 1, 2020

ljharb commented Sep 1, 2020

ljharb commented Sep 1, 2020

mmarchini commented Sep 1, 2020

mscdex commented Sep 1, 2020

ljharb commented Sep 1, 2020

Trott commented Sep 4, 2020

addaleax commented Sep 7, 2020

ljharb commented Sep 7, 2020

addaleax commented Sep 7, 2020

ljharb commented Sep 7, 2020

mmarchini commented Sep 7, 2020

richardlau commented Sep 7, 2020

ljharb commented Sep 7, 2020

mmarchini commented Sep 7, 2020

build: require "allow edits" to be checked #35002

build: require "allow edits" to be checked #35002

Conversation

ljharb commented Aug 31, 2020 • edited Loading

mmarchini Aug 31, 2020

Choose a reason for hiding this comment

ljharb Aug 31, 2020

Choose a reason for hiding this comment

ljharb Aug 31, 2020 • edited Loading

Choose a reason for hiding this comment

mmarchini Aug 31, 2020

Choose a reason for hiding this comment

mscdex commented Aug 31, 2020

ljharb commented Aug 31, 2020

mscdex left a comment

Choose a reason for hiding this comment

mmarchini commented Sep 1, 2020

ljharb commented Sep 1, 2020

mscdex commented Sep 1, 2020

ljharb commented Sep 1, 2020

mmarchini commented Sep 1, 2020 • edited Loading

mmarchini commented Sep 1, 2020

ljharb commented Sep 1, 2020

ljharb commented Sep 1, 2020

mmarchini commented Sep 1, 2020

mscdex commented Sep 1, 2020

ljharb commented Sep 1, 2020

Trott commented Sep 4, 2020

addaleax commented Sep 7, 2020

ljharb commented Sep 7, 2020

addaleax commented Sep 7, 2020

ljharb commented Sep 7, 2020

mmarchini commented Sep 7, 2020

richardlau commented Sep 7, 2020

ljharb commented Sep 7, 2020

mmarchini commented Sep 7, 2020

ljharb commented Aug 31, 2020 •

edited

Loading

ljharb Aug 31, 2020 •

edited

Loading

mmarchini commented Sep 1, 2020 •

edited

Loading