✨ Use pull_request_target + protected env for e2e

[azeemshaikh38]

• Please check if the PR fulfills these requirements

• Tests for the changes have been added (for bug fixes / features)
• PR title follows the guidelines defined in https://github.com/ossf/scorecard/blob/main/CONTRIBUTING.md#pr-process

• What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

• What is the current behavior? (You can also link to an open issue here)

• What is the new behavior (if this is a feature change)?

• Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

• Other information:

[laurentsimon]

did you try using workflow_run as suggested by someone? Why can we not use it?

[azeemshaikh38]

did you try using workflow_run as suggested by someone? Why can we not use it?

For workflow_run we pass some context from pull_request (or any other non-secret accessing workflow) to the workflow_run job where it will have access to secrets. In our case, this doesn't work because we will most likely need to pass a pre-built binary and use that in workflow_run which is not safe.

[azeemshaikh38]

I can't get the integration test workflow to run in this PR. How do we test changes to such workflows?

[laurentsimon]

I can't get the integration test workflow to run in this PR. How do we test changes to such workflows?

the code used by GitHub is the one in the target/base branch, IFAIK. You'll have to merge into main to test it out. Maybe try it on a cloned repo?

[azeemshaikh38]

I can't get the integration test workflow to run in this PR. How do we test changes to such workflows?

the code used by GitHub is the one in the target/base branch, IFAIK. You'll have to merge into main to test it out. Maybe try it on a cloned repo?

So I setup my fork to have this change - https://github.com/azeemshaikh38/scorecard/blob/main/.github/workflows/integration.yml. Could you send my fork a PR from your fork to test this?

[laurentsimon]

I can't get the integration test workflow to run in this PR. How do we test changes to such workflows?

the code used by GitHub is the one in the target/base branch, IFAIK. You'll have to merge into main to test it out. Maybe try it on a cloned repo?

So I setup my fork to have this change - https://github.com/azeemshaikh38/scorecard/blob/main/.github/workflows/integration.yml. Could you send my fork a PR from your fork to test this?

you can create PRs to you fork, right? Does it make a difference if I send the PR or you do?
I want to be sure you can iterate on sending more PRs for testing, and not be blocked by me to resend, etc

[azeemshaikh38]

you can create PRs to you fork, right? Does it make a difference if I send the PR or you do? I want to be sure you can iterate on sending more PRs for testing, and not be blocked by me to resend, etc

You are right I was able to test it out -

I think this PR is good to submit.

[laurentsimon]

LGTM overall. I don't think this is the right approach because it affects productivity so much. Every time we submit a PR, we'll have to wait for someone to LGTM to run the integration test.

As an interim solution, it's better than having PR blocked for ever so let's merge in.

I think the right approach is to use workflow_run.

wdut?

[laurentsimon]

see comment about printed message. I don't understand how we know it's been reviewed.

[azeemshaikh38]

see comment about printed message. I don't understand how we know it's been reviewed.

See #1308 (comment)

[laurentsimon]

Thanks Azeem. Let's merge this one in and merge all the blocked PRs. Do you agree that we should move towards workflow_run to avoid affecting productivity? (In a follow-up PR)

[azeemshaikh38]

Thanks Azeem. Let's merge this one in and merge all the blocked PRs. Do you agree that we should move towards workflow_run to avoid affecting productivity? (In a follow-up PR)

No workflow_run will not work for us. At least, AFAICT. We can build the binaries in pull_request and pass it to workflow_run so that it can use the secret token, but then the binary itself will be a security threat.

[azeemshaikh38]

This is still stuck :( I'm going to try and force submit it. Hopefully it should fix the issue for the other PRs.

[laurentsimon]

Thanks Azeem. Let's merge this one in and merge all the blocked PRs. Do you agree that we should move towards workflow_run to avoid affecting productivity? (In a follow-up PR)

No workflow_run will not work for us. At least, AFAICT. We can build the binaries in pull_request and pass it to workflow_run so that it can use the secret token, but then the binary itself will be a security threat.

I see, that makes sense. Let's see if it's usable or not. Otherwise let's move to a model where we run the e2e tests without admin token. My understanding is that the token requires admin for certain check like branch protection: is my understanding correct? If that's the case, maybe we could at least separate the branch protection from the rest, to avoid blocking PR authors.

@naveensrinivasan has allowed/approved many PRs to unblock us, but it was not actually a code review: there was no LGTM. What I'm concerned with is that we're going to approve in a rush to allow the tests to run, without doing a proper review. @naveensrinivasan how is the user experience to approve these test to run?
I'll try to go thru it on my end to test it more.

[naveensrinivasan]

TBH this is painful!