Revert required certify safe

[mcollina]

Fixes #801

[mcollina]

I think this is too restrictive and a different implementation should be reached.
This is blocking collaborators in starting CI using the flag, essentially nullifying the usefulness of the flag itself.

(I was wrong in LGTMing this, sorry)

[mcollina]

cc @joyeecheung @nodejs/tsc

[aduh95]

I'd prefer if we find another solution than to revert, maybe we could improve the CLI flow for starting CI.

[MoLow]

I think we should wait before landing this to discuss with TSC since #801 is on the agenda.
I wonder if there is a way/GitHub API to check which user added the label to a PR, then we can check if the user who added the label is a collaborator

[targos]

We could have an action that automatically removes the label if it was applied by someone who's not allowed.

[aduh95]

We could have an action that automatically removes the label if it was applied by someone who's not allowed.

There would be a race condition though, I don't think that's a sound solution. The GHA workflow could add --certify-safe on PRs opened by collaborators – but I still think there would be value in nudging folks into using the CLI flow.

[mcollina]

There would be a race condition though, I don't think that's a sound solution. The GHA workflow could add --certify-safe on PRs opened by collaborators – but I still think there would be value in nudging folks into using the CLI flow.

I do most of my CI-nudging on mobile :(

[MoLow]

but I still think there would be value in nudging folks into using the CLI flow.

what is that value? and in that case, why won't I access https://ci.nodejs.org/ and trigger a job manually?
I think there is a huge value in performing this via Github UI. (I use mobile a lot as well)

[aduh95]

what is that value?

There would be incentive to improve the CLI, while keeping for triaggers the ability to run CI on approved PRs – and only approved PRs.

why won't I access https://ci.nodejs.org/ and trigger a job manually?

It's always been a possibility, nothing changes on that front (no matter if this revert lands or not, no matter if the CLI improves or not).

I think there is a huge value in performing this via Github UI

It's still there – only for fewer PRs, the one that have been approved.

(I use mobile a lot as well)

Surely you don't send your PRs from mobile do you? If you mean reviewing PRs, how often does it happen that you would want to run CI before you approve it? I suppose it would depend on the type of changes that's being suggested, and maybe we have different experiences around it, the current settings were chosen because it matched my personal "kicking off CIs" style.

[mcollina]

If you mean reviewing PRs, how often does it happen that you would want to run CI before you approve it?

I would guess 80-90% of the time.

I suppose it would depend on the type of changes that's being suggested, and maybe we have different experiences around it, the current settings were chosen because it matched my personal "kicking off CIs" style.

Fair enough, but we should be supportive of others too.

[mcollina]

I think we should wait before landing this to discuss with TSC since #801 is on the agenda.

I would prefer to not loose another week.

[ShogunPanda]

I also incurred into this. This is definitely not what we want.

[marco-ippolito]

Another annoying issue is when your pr is approved, you amend a nit and you cannot start CI, without asking to re-approve the PR

[aduh95]

Reverting this is not the right move IMO, if you'd like to unblock the situation, you should open a PR to make the action pass --certify-safe label on nodejs/node – or make it use the older version of ncu. Reverting this is going to make it harder to iterate on it.

[mcollina]

I don't have much time to iterate on this atm. I think reverting gives us the quickest path forward to restore functionality.

[mcollina]

Where should the option be passed?

[aduh95]

Where should the option be passed?

When calling nci-ci – that bypass the check for approvals.