Reduce recurring steps in CI workflow

[nineinchnick]

Description

Extracted from #12817

Non-technical explanation

n/a

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

[MiguelWeezardo]

Shouldn't the caching from #14882 be part of the composite action used here?

[nineinchnick]

Shouldn't the caching from #14882 be part of the composite action used here?

Yes, that's why I opened trinodb/github-actions#15 and I'll update the commit sha here when necessary.

[MiguelWeezardo]

Make sure to update the checkout-and-setup tag to something which uses actions/cache explicitly.

[nineinchnick]

I also extracted uploading test results and reports into a separate action and added a step to grab heap dumps.

[findepi]

@nineinchnick
who can access the heap dumps? where are they stored and for how long?

[nineinchnick]

Ah, I forgot that they can contain sensitive data, like secrets. If we upload them as artifacts, then they're public. I'll add a condition to only do this if there are no secrets set.

[findepi]

This will work for now. If we have OOMs with secrets-requiring jobs, we can perhaps encrypt those dumps.

[findepi]

Let's try to simplify.

Can we somehow explicitly detect the PR is not trusted?
If github doesn't make it explicit, we could check that the build is a PR and the source repo != trinodb/trino

wdyt?

(i am worried that addition of a new secret will be overlook here)

alternatively, we could have a dummy secret SECRETS_PRESENT in trinodb/trino
and condition on that here

[nineinchnick]

alternatively, we could have a dummy secret SECRETS_PRESENT in trinodb/trino
and condition on that here

I like this idea. I didn't try to add a condition to detect forks because I'm not sure we wouldn't miss some cases. Checking for secrets seemed more direct.

[nineinchnick]

Can you create such a secret? Or would it be enough to check for just one of the existing secrets, with a comment?

[findepi]

I created org-wide SECRETS_PRESENT secret.

[hashhar]

why is checking for PR source not enough? i.e. source repo != trinodb/trino?

[findepi]

I don't understand commits other than "Attempt to upload heap dump if tests failed".

@hashhar do you want help reviewing?
@nineinchnick would be possible to decouple? (seems not obvious, but maybe...)

[nineinchnick]

@nineinchnick would be possible to decouple? (seems not obvious, but maybe...)

Decoupling would mean I'd have to do this twice, once by duplicating it 4 times and then again in this PR. Would it help if I'd extract it into another PR based on this branch?

[findepi]

Decoupling would mean I'd have to do this twice, once by duplicating it 4 times and then again in this PR.

sounds bad
how bad?

Would it help if I'd extract it into another PR based on this branch?

no, because i still wouldn't be able to merge this :)

[hashhar]

upload-heap-dump should be very carefully used - I'm not strongly in favour of enabling it by default. We should enable it when debugging for a specific job and not always.

Also are the heap dump artifacts only visible to people with write access to repo? If not then it's not safe to do at all.

[hashhar]

stringly typed booleans create confusion specially when the value being used can be easily confused for a boolean

[hashhar]

why is checking for PR source not enough? i.e. source repo != trinodb/trino?

[hashhar]

Let's extract last 2 commits and merge the first one. the last 2 are controversial.

[nineinchnick]

Let's extract last 2 commits and merge the first one. the last 2 are controversial.

Done, I'll open up another PR after this one gets merged.

[nineinchnick]

upload-heap-dump should be very carefully used - I'm not strongly in favour of enabling it by default. We should enable it when debugging for a specific job and not always.

If we want to use it to debug OOM errors that happen sporadically, it'll not be very useful. I wonder if it would make more sense to try to reproduce the OOM locally, for ex. in a container with limited memory.

Also are the heap dump artifacts only visible to people with write access to repo? If not then it's not safe to do at all.

Why they're not safe? Do you have ideas for any alternatives?

[hashhar]

Also are the heap dump artifacts only visible to people with write access to repo? If not then it's not safe to do at all.

Why they're not safe? Do you have ideas for any alternatives?

I now see that you only upload them if secrets don't exist so that solves my concern.

[hashhar]

LGTM.

[hashhar]

LGTM % check-commits can also use .github/actions/setup it seems.

[nineinchnick]

@hashhar all green!