Updating GitHub Actions to new Node20 versions

[davidorme]

Description

Updates actions to new versions using Node20, not deprecated Node16

Fixes #383

Type of change

• New feature (non-breaking change which adds functionality)
• Optimization (back-end change that speeds up the code)
• Bug fix (non-breaking change which fixes an issue)

Key checklist

• Make sure you've run the pre-commit checks: $ pre-commit run -a
• All tests pass: $ poetry run pytest

Further checks

• [-] Code is commented, particularly in hard-to-understand areas
• [-] Tests added that prove fix is effective or that feature works

[jacobcook1995]

Still getting a couple of warnings (for actions/cache@v3 and codecov/codecov-action@v3), don't know if those have newer versions? I don't think we use cache directly though?

[davidorme]

Turns out the pre-commit uses cache but the pre-commit action is in maintenance only mode and they will not "fix" this as they say it should just keep working?

pre-commit/action#189

The pre-commit authors basically want us to switch to pre-commit.ci instead, but this doesn't seem urgent.

[davidorme]

codecov is on its way with version 4, but it seems there are some teething issues:
codecov/codecov-action#1248

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (76f1906) 93.18% compared to head (36fed75) 93.18%.

❗ Current head 36fed75 differs from pull request most recent head 2f3f5e9. Consider uploading reports for the commit 2f3f5e9 to get more accurate results

Additional details and impacted files

@@           Coverage Diff            @@
##           develop     #384   +/-   ##
========================================
  Coverage    93.18%   93.18%           
========================================
  Files           60       60           
  Lines         2920     2920           
========================================
  Hits          2721     2721           
  Misses         199      199           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jacobcook1995]

Hmm yeah that seemed to break codecov probably best to leave for now then.

[davidorme]

@dalonsoa Have you handled this switch from the pre-commit/action to using precommit.ci ? Seems like we're going to have to move across at some point - might as well be now if its an easy and uncontroversial switch, but it doesn't seem like the Node16 deprecation issue is critical.

#384 (comment)

[dalonsoa]

I seem to remember it was pretty easy. We use it in this project (you can see the status badge there), but forgot to disable the pre-commit action, so have both running. But, to be honest, we have not been consistent and in other projects we have forgotten about it - among other things because it does not work (without paying) for private repos, and we have a few of those.

[davidorme]

I've got pre-commit.ci working. There doesn't seem to be a mechanism for making the test and docs_build jobs reliant on pre-commit.ci - they all just run in parallel? I kinda like that the code qa passes before bothering with anything else...

[dalonsoa]

I think that's way we left it in there. But yeah, pre-commit.ci is a totally independent tool run in parallel.

Having said that, and totally underelated, the tests with Ubuntu and Python 3.10 are a bit flaky, right? They fail for no apparent reason and pass if re-run. Any idea of why that might be?

[davidorme]

Having said that, and totally underelated, the tests with Ubuntu and Python 3.10 are a bit flaky, right? They fail for no apparent reason and pass if re-run. Any idea of why that might be?

Yeah - that's bothering me too.

[davidorme]

OK - so vr_run is failing with the odd error that a variable is missing. The problem is that this is hard to debug without the log, which is in a tempdir on the runner. If we update that test to export the VR_RUN_TEMPDIR_PATH, we could then cache the contents using the artefacts action?

[davidorme]

Ah - no. The test uses:

    with TemporaryDirectory() as tempdir:
        try:

I think that gets deleted when the context handler exits? Any other thoughts @dalonsoa

[dalonsoa]

Maybe this is pointing to an interesting situation: the error message is not clear enough to suggest what's wrong and only checking the logs, which are not always available or within easy reach, you can have an idea of the problem. It is also a problem that the try... block covers a lot of things that can go wrong.

Aside from providing the full error log as output from running the tests, I cannot think of anything else.

[davidorme]

@dalonsoa This doesn't fix the wider issue of vr_run being too monolithic, but it looks like I've fixed that platform bug successfully. Can you approve - assuming you do!

[dalonsoa]

I do :)