Fix the Ormolu auto-format GH workflow step

[sellout]

Overview

The Ormolu check in CI was not working. Now it does.

Implementation notes

There were a few issues:

• The previous step (which collects the names of changed files) was missing its id, so the Ormolu step couldn’t retrieve the list of changed files.
• That same step was also escaping the newline separator cancelling it out.
• The job was passing, even if there was unformatted code, if it couldn’t commit the changes.

Interesting/controversial decisions

Fixing the first two issues is pretty straightforward, but the third one isn’t.

The job can’t create a commit on either protected branches, or on a fork. So, third-party contributors & their reviewers get no feedback that their code isn’t formatted correctly. However, the job runs on both PRs and pushes. The push job actually runs on the fork itself, and thus can create the commit.

So, you push everything once, and make a PR? Bob’s your uncle – the autoformatting gets done on your push job and the PR job either silently fails to format or succeeds because the automated commit has already happened.

Except … if you push incrementally, or add additional commits after the initial push, the push job will only look at the last commit (not even the entire push), whereas the PR job always looks a the entire PR. So, the confusing situation I found myself in was that when I first put up the PR, the auto-commit happened (based on my push, not the PR), but later commits didn’t have the same effect, and the PR jobs (which are the only ones I realized were happening) just skipped the “push a commit” step.

So, the way I finally did it is to run Ormolu inplace and make the commit if possible, but to otherwise run Ormolu check and fail the job if formatting was incorrect. From that, I expect contributors to see one of these behaviors:

1. they push a single commit, the job runs and adds the formatting commit, then they open the PR, and it passes;
2. they push a single commit, open the PR before the job runs, and the PR fails its Ormolu check, but then the fork adds the formatting commit, triggering another PR run, which then passes; or
3. they push more than one commit, any formatting changes in files modified in the last commit get auto-fixed, but changes in other files don’t, and the PR’s run catches those discrepancies and stays failing.

It’s not perfect, but it at least seems better than the previous situation that would just always pass and never make any changes.

Test coverage

This PR introduces a mis-formatting to trigger the auto- formatting in order to test the workflow change. It doesn‘t actually work that way though (see the previous section), so there is a final commit that fixes the build based on the failing ormolu check in CI.

Loose ends

Hopefully there’s a better way to manage the auto-commit thing. I would think having “Allow edits and access to secrets by maintainers” on the PR would be enough to allow the auto-commit to happen on the PR in some way, and having this done on pushes is probably wrong (but doing the rest of CI on pushes is probably nice, and we don’t want to throw out the baby with the bathwater) – so maybe the ormolu job should just be in a separate workflow or something?

[aryairani]

Oh, thanks. I was subconsciously wondering about this.

[aryairani]

Whoops!

[aryairani]

I can't remember why this was here, but we may discover a reason later. It probably belongs somewhere else if anywhere,

[sellout]

Yeah, I couldn’t figure out why it would be useful to run this step if an earlier one failed.

[aryairani]

Can we detect when a push has multiple commits, and then run a full ormolu instead of the currently filtered to the files changed in the commit? Or maybe filter to the files changed in the PR?

[aryairani]

2. they push a single commit, open the PR before the job runs, and the PR fails its Ormolu check, but then the fork adds the formatting commit, triggering another PR run, which then passes; or

I don't think automatic commits trigger another run unfortunately. Am I wrong? Although it's okay as long as we're careful to check which tests ran before merging a PR

[sellout]

Can we detect when a push has multiple commits, and then run a full ormolu instead of the currently filtered to the files changed in the commit? Or maybe filter to the files changed in the PR?

I don’t know what kind of introspection we have.

I don't think automatic commits trigger another run unfortunately. Am I wrong? Although it's okay as long as we're careful to check which tests ran before merging a PR

No, you’re right – I always forget that.

I feel like having this job run on push is maybe just the wrong thing. Doing either check or inplace on the PR seems good enough. If a reviewer sees that it’s the Ormolu check that’s failing, they can run ormolu and push to the PR. It’s not perfect, but I think it’s a lot clearer. Maybe scripts/check.sh can also run ormolu if it finds it on the path, to help contributors notice before pushing?

[aryairani]

Can we detect when a push has multiple commits, and then run a full ormolu instead of the currently filtered to the files changed in the commit? Or maybe filter to the files changed in the PR?

I don’t know what kind of introspection we have.

I don't think automatic commits trigger another run unfortunately. Am I wrong? Although it's okay as long as we're careful to check which tests ran before merging a PR

No, you’re right – I always forget that.

I feel like having this job run on push is maybe just the wrong thing. Doing either check or inplace on the PR seems good enough. If a reviewer sees that it’s the Ormolu check that’s failing, they can run ormolu and push to the PR. It’s not perfect, but I think it’s a lot clearer. Maybe scripts/check.sh can also run ormolu if it finds it on the path, to help contributors notice before pushing?

I'm good with trying any/all of that — is there anything I should do or just sit tight?