Enable required workflows?

[IndrajeetPatil]

@rempsyc brought up a good point about making success of some workflows to be a required condition before a PR can be merged. This is to add an extra speed bump to prevent contributors with admin privileges from merging PRs that don't pass even the most basic quality assurance check. For more, see docs on required workflows.

I would propose (if you need a refresher on which workflows we use and why, see workflows summary) the following workflows can be deemed to be required:

• R-CMD-check-hard
• pkgdown

I don't want R-CMD-check to be a required workflow because it is quite fragile and can be easily broken by changes in any of our hundreds of soft dependencies.

WDYT?

[IndrajeetPatil]

P.S. Needless to say, everyone with admin privileges can still merge the PR even if the required workflows are failing.

[mattansb]

I thought this was already the case 😅

[rempsyc]

Thanks Indra. Indeed, it is merely an extra psychological speed bump for merging, as one must make the conscious decision to bypass branch protections. When 50% of our workflows are failing because of errors with Matrix or installation of other packages or some environment setup error, it can be tempting to force a merge assuming that all of the workflows fail for similarly trivial reasons.

However, I think we agree that some workflows are more important than others and we should make sure they pass as most basic quality assurance checks. You propose R-CMD-check-hard and pkgdown, and I think those are reasonable.

For the repo I maintain, report, I had also made lint-changed-files a required workflow. The reason for this is that otherwise lints will never get done (nobody ever fixes the lints basically) and therefore the workflow again essentially becomes useless. Now that's fine and I undertand fixing lints is annoying, but I proposed to fix them myself on the PRs for report so that in 15 years or so the whole package will eventually pass the lint check 😈

However, that means the PR author should not merge their PR until I had time to fix the lints, and this where making it a required workflow comes in for report. However, I don't think it is realistic to add this to all our repos. Indra I suppose you don't want some repos to have extra required workflow permissions for special cases such as this?

[IndrajeetPatil]

I am fine with individual maintainers calling shots on whether the lint-changed-files should be a required workflow. Sometimes fixing an important issue is far more time sensitive than fixing lints, and I can see how one might choose to bypass it in such cases.

otherwise lints will never get done (nobody ever fixes the lints basically)

Btw, we have been cleaning lints here and there (well, most prominently Daniel for the massive repos he maintains 🙈), but the sheer volume of our codebase makes it quite the Herculean task. We should get there in a couple of years 🤞

[rempsyc]

Yes, it's more of a small nudge; as you've highlighted before, in cases where there is a time sensitive issue, it is always possible to bypass required workflows (including lint-changed-files) and force a merge.

[etiennebacher]

This is to add an extra speed bump to prevent contributors with admin privileges from merging PRs that don't pass even the most basic quality assurance check

P.S. Needless to say, everyone with admin privileges can still merge the PR even if the required workflows are failing.

I'm confused about the goal of setting "required" workflows if we can bypass them. We already have a big red text saying something along the lines of "Merge now and bypass necessary approval", so why would setting some workflows as required add a nudge? Or does it generate a message saying something like "Careful: workflow pkgdown didn't pass"?

Finally, in the github docs for required workflows, they say:

Note: GitHub no longer supports required workflows for GitHub Actions. To require workflows to pass before merging, upgrade your GitHub Enterprise Server instance to the latest version and use repository rulesets instead.