.github/workflows/editorconfig.yml: Don't use GitHub API for PR diff.

[mohe2015]

This caused ratelimits for large PRs (reformatting PRs)

Motivation for this change

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 21.11 Release Notes (or backporting 21.05 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
  • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
• Fits CONTRIBUTING.md.

[zowoq]

This caused ratelimits for large PRs (reformatting PRs)

Can you list some PRs where this happened?

Test at ...

What about testing some large PRs?

[mohe2015]

This caused ratelimits for large PRs (reformatting PRs)

Can you list some PRs where this happened?

#147608 see https://github.com/NixOS/nixpkgs/runs/4342305588?check_suite_focus=true though we're not sure if the ratelimit is caused by the editorconfig specifically. It seems like it does quite a few requests though.

Test at ...

What about testing some large PRs?

Will do when github is more responsive. Need to do this on a fork as you need the updated workflow files and it's really annoying.

[mohe2015]

Also #146798 https://github.com/NixOS/nixpkgs/runs/4275079944?check_suite_focus=true
Note that this also affects PRs that are open at the same time as the ratelimit is global. There were complaints about that in the Matrix https://app.element.io/#/room/#infra:nixos.org channel afterwards. The other PR I linked was supposed to circumvent that with the [skip ci] but unfortunately that didn't work.

[0x4A6F]

This caused ratelimits for large PRs (reformatting PRs)

Can you list some PRs where this happened?

Without the canceled EditorConfig jobs, then #147623 and #147608 would cause rate limiting.

It failed in #121490 run and #146798 run.

[mohe2015]

mohe2015#14 is a test of a big formatting PR. I also fixed a second bug - namely that the argument list got too big (the github action environment variable actually).

[zowoq]

What about PRs with only deleted file(s)? The if: env.PR_DIFF conditions that have been removed were a guard to cover that case.

[mohe2015]

New tests at mohe2015#16 and mohe2015#17 and mohe2015#18

Diff between base is just the if deleted mohe2015/nixpkgs@editorconfig-prevent-ratelimits...mohe2015:editorconfig-prevent-ratelimits-base

[mohe2015]

What about PRs with only deleted file(s)? The if: env.PR_DIFF conditions that have been removed were a guard to cover that case.

Should be fixed by --no-run-if-empty

[zowoq]

Sorry, this is now obscuring the actual editorconfig-checker command.

I intentionally made it so it was visible and easily understood without needing to click to expand anything.

[mohe2015]

Sorry, this is now obscuring the actual editorconfig-checker command.

I intentionally made it so it was visible and easily understood without needing to click to expand anything.

So I should switch back to the "pipe a file" method?

So it's cat $HOME/changed_files | xargs --no-run-if-empty editorconfig-checker -disable-indent-size ?

And should I use a shorter input file name? Unfortunately there doesn't even seem to be any documentation which files are persisted between steps.

Also I should probably use the shorthand -r for --no-run-if-empty ?

Feel free to push that change to this branch - I will maybe be able to do this tomorrow but not sure.

Also thank you for the nice reviews.

[zowoq]

cat $HOME/changed_files | xargs -r editorconfig-checker -disable-indent-size should be fine.

[mohe2015]

@zowoq done the requested change and updated the test PRs

[zowoq]

Thanks @mohe2015!

[0x4A6F]

Thanks, now crunching on #147623 with succes. ❤️

[zowoq]

Reverting in #147959.

Causing errors:
https://github.com/NixOS/nixpkgs/runs/4360925051?check_suite_focus=true
https://github.com/NixOS/nixpkgs/runs/4360946550?check_suite_focus=true
https://github.com/NixOS/nixpkgs/runs/4360965902?check_suite_focus=true
https://github.com/NixOS/nixpkgs/runs/4360989140?check_suite_focus=true

[0x4A6F]

Some failures due to at least renames.

Could be also reversed to exclude at least deleted and renamed: --diff-filter=dr

[mohe2015]

I was wondering how long it would take until something breaks ...

[mohe2015]

Do we maybe have to compare the filelist of the current master / commit into which to merge with the merge commit? And then only check these files?

[zowoq]

These sorts of problems is why I switched to using the simpler github API.

I think if we're going to try this again it will need to be tested much more prior to merging so we don't unnecessarily annoy other contributors.

[mohe2015]

These sorts of problems is why I switched to using the simpler github API.

Now I see :D

I think if we're going to try this again it will need to be tested much more prior to merging so we don't unnecessarily annoy other contributors.

I was not awake at merge time otherwise I would've looked too. But it seems like you resolved it pretty quickly.

There is really bad documentation on which of the branches is what at which time (e.g. does the base branch get updated on check run or when?) etc.

I was just testing a bit and I think pretty weird shit happens additionally: The base branch of mohe2015#21 only contains mohe2015@a6e4931 but to me it seems like it cat's the changed_files file in https://github.com/mohe2015/nixpkgs/runs/4374828551?check_suite_focus=true although I only added that change in a different branch. Maybe I'm looking wrong but to me this looks incorrect. (The output is interleaved for some reason)

I'm pretty sure though I found a mistake I just need to find out how to fix it.

Also annoying to run the checks as you have to rebase and push and stuff.

[mohe2015]

Ohh nice github, clicking the "View workflow file" shows a totally different workflow which doesn't contain the "-r" yet

[zowoq]

But it seems like you resolved it pretty quickly.

Yes, this time.

I spent more time than I care to think about testing it and then monitoring it for months after if was merged to ensure it worked reliably.

[zowoq]

I appreciate that you're taking the time to try and fix this but giving how rarely the actual problem that you want to resolve occurs (API issues caused by massive PRs) another attempt is going to need to be tested very thoroughly before I would be willing to merge it.

[mohe2015]

What is the reason we don't just fix all editorconfig issues in this repository and then just always check all files? As far as I remember there weren't too many

[mohe2015]

The better solution is probably to add a [CI skip] check and call it a day

[zowoq]

What is the reason we don't just fix all editorconfig issues in this repository and then just always check all files? As far as I remember there weren't too many

Yeah, this would be easier but we don't want to start failing every PR because one bit of whitespace slipped in, e.g because it was pushed straight to master or reintroduced via a reverted PR. We may be able to do this in the future (required checks, protected branches, merge bots, etc) but I don't see that it is feasible at the moment.

The better solution is probably to add a [CI skip] check and call it a day

This would likely be easier to do. IIRC Github handles it automatically for push and pull_request events but we use pull_request_target so we don't need to approve and then re-approve subsequent action runs on PRs from first time contributors to this repo (of which I discovered there are actually a lot of).

If you like I'm happy to do it myself but I probably won't get around to it for a few days.

[mohe2015]

If you like I'm happy to do it myself but I probably won't get around to it for a few days.

That would be great! Thanks again for all the help.

[zowoq]

No problem, thanks anyway for you work on this.

I'll try to reuse the $HOME/changed_files method as that did address another bug.

https://docs.github.com/en/actions/managing-workflow-runs/skipping-workflow-runs

• [skip ci]
• [ci skip]
• [no ci]
• [skip actions]
• [actions skip]

Alternatively, you can end the commit message with two empty lines followed by either skip-checks: true or skip-checks:true.

As this only needs to be implemented for the editorconfig check (and I guess we may be able drop it after the formatting PRs are merged) I might just use [skip editorconfig], trying to match all of the github conditions will be a bit tedious and also using a generic [skip ci] may be a bit confusing for contributors. ofborg has its own behaviour for skipping builds as well just to make it more confusing.

Using a label may also be possible but it's probably not a good idea as it would need to be applied before the PR is opened to avoid the API issue.

[zowoq]

I'll try to reuse the $HOME/changed_files method as that did address another bug.

May as well do this separately in case the skip ci thing turns out to be problematic for whatever reason.

PR: #148099

[zowoq]

I think this will likely be the easiest option:

diff --git a/.github/workflows/editorconfig.yml b/.github/workflows/editorconfig.yml
index 01a6d736c13..f7c96a7dd20 100644
--- a/.github/workflows/editorconfig.yml
+++ b/.github/workflows/editorconfig.yml
@@ -11,7 +11,7 @@ on:
 jobs:
   tests:
     runs-on: ubuntu-latest
-    if: github.repository_owner == 'NixOS'
+    if: "github.repository_owner == 'NixOS' && !contains(github.event.pull_request.title, '[skip editorconfig]')"
     steps:
     - name: Get list of changed files from PR
       env:

Checking commit messages becomes complicated when there is more than one commit unless we require that the skip be set in e.g. the HEAD commit.

Also for git history it may be better to avoid cluttering commit messages with CI directives?

[zowoq]

PR: #148327