[ci] Cleanup caches in the target repo

[ntadej]

We need to use pull_request_target so that caches from forks are deleted. We only request relevant permissions so this should be safe enough.

Also quote variables, just in case.

Why delete caches? We cache a lot of data and the quota we have is low. This prevents relevant caches to get evicted too soon.

[ntadej]

We may want to clear all caches once this is merged. I can do that if fine with you @louwers.

[codecov]

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (dc77b6d) 85.78% compared to head (6f86525) 85.80%.
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1758      +/-   ##
==========================================
+ Coverage   85.78%   85.80%   +0.02%     
==========================================
  Files         566      566              
  Lines       27815    27881      +66     
==========================================
+ Hits        23860    23923      +63     
- Misses       3955     3958       +3     

see 4 files with indirect coverage changes

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

[louwers]

The Qt workflows are using up 4260MB of cache storage for a single workflow run, almost half of the 10GB cache limit. So even if this is merged relevant caches probably will continue to get evicted too soon.

Let's solve that problem first... One idea is to use an S3 Cache:

https://github.com/marketplace/actions/s3-cache-for-github-actions

We would save only on main (because a secret is needed) but retrieve with a public read-only key.

I'm not sure if I like removing caches from PRs because when workflows get skipped on main (ideally almost always when a workflow already ran for a PR with the same code) we'd potentially end up with a stale cache.

[ntadej]

Yes, I will disable the debug symbols.

Still now all PR caches stay there. I see no reason why this would not get merged. It will never affect the caches of the branch merged into.

[ntadej]

To add, we already evict caches for PRs open from branches directly in this repo. It's just the forks that do not work.

[louwers]

It will never affect the caches of the branch merged into.

If a PR adds a node module (as an example), it will create a cache for it, then when it is merged the workflow is skipped on main and the cache is deleted. A subsequent PR won't have a cache of the node modules.

If you still want to merge this we can, but we should not check out any code in a pull_request_target workflow, so please remove that step.

[ntadej]

PRs only use caches from main or their own caches. main will always recreate caches (I guess to avoid getting some malicious data from PRs). At least this is my experience.

AFAIK we need the clone for GitHub CLI to work, but I can try to workaround it. Also I see no security issue in cloning what was just merged in main because this runs after merge so as long nothing malicious is merged, we are fine.

Regarding ccache size, I think we accumulate historic data each time the code changes. We need to set some sensible limit e.g. 200 MB (or maybe even lower). I will look into it.

[louwers]

A pull request can be closed by the author without being approved and merged.

[ntadej]

Sure, and then we should also delete the caches as they are irrelevant. As far as I understand this will still take the action from the main branch so we should not have a problem (but we can test it).

[louwers]

"you should make sure that you do not check out (...) untrusted code"

https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#pull_request_target

But not sure what the harm is as long as we don't do anything with it.

[ntadej]

Will close (and reopen) this MR to see what happens.

[ntadej]

Hmm, nothing happened. I guess this is good from security point of view, although not solving the caches issues 😊

Let's merge this for now and monitor the behaviour.