Allow the regular CI pipeline to be run on external PRs

[tgummerer]

Currently when an external contributor submits a PR, we need to kick of the CI pipeline manually. Doing that runs basically the same tests as the regular CI pipeline, except it's easy for it to get out of sync.

The only advantage of doing it this way is that we can review the PR before running the pipeline. However the way our GitHub repo is set up, we already need to approve PRs before CI is being run.

Unify the CI pipeline, which allows for the status to be reported on the PR, making the experience for external contributors a bit nicer, while still requiring us to approve running the CI jobs first but now just with the click of a button.

[pulumi-bot]

Changelog

[uncommitted] (2024-04-29)

[tgummerer]

Docs for how this GitHub feature works: https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks

[justinvp]

Blocked on internal discussion

[tgummerer]

Summarizing our internal discussion here a bit. Our biggest concern is that users could exfiltrate secrets. There's a couple of things that mitigate that risk:

• There need to be approvals (in the GitHub UI) for any changes by external contributors (a little more on that later)
• With the pull_request target the workflows don't actually get access to secrets

The latter issue complicates things a little bit, since even with approvals users don't have access to secrets.

On the other hand the pull_request_target and workflow_run do allow access to secrets. workflow_run is what we're using right now, but pull_request_target is automatically run without any approvals, so it's probably not what we want due to the risk of exfiltrating secrets.

So for this repo I would propose the following:

• Set the approval setting to "Require approval for first-time contributors". This means first time contributors would still need approval, so they couldn't use our CI minutes for mining bitcoin or whatever, but after they made the effort to come up with a contribution that we consider worthy of running CI on workflows are started automatically. This gives a much better contributor experience.
• Since secrets are no longer guaranteed to be present in CI, we need to make sure to have appropriate t.Skip()s for tests that do require secrets. We're only giving the contributors a chance to run the tests that do not require any secrets. We're trying to do this right now already, but there's probably some cleanup necessary. This also helps local dev a bit since not all secrets are set all the time in that environment. So we should be able to run tests successfully without secrets anyway.
• We keep the additional /run-acceptance-tests slash command for running all the tests with secrets. This will be an optional step, since the merge queue will run the tests correctly including the secrets either way.

While this doesn't give us as much of a cleanup, I think it provides a much better contributor experience, especially as folks will be able to see if CI on their PR failed pretty immediately.

Thoughts? /cc @blampe @AaronFriel

[AaronFriel]

My understanding from what @blampe described is that apparently, external forks run without secrets. Which is a great default, but it likely means that the meaningful work will be in fixing tests to run offline mode.

For integration tests, a good deal of that was done in #10720. Our provider repositories are much harder to test without access to the cloud provider, but for the pulumi/pulumi repository, we should be able to run every test offline.

I'd like to thoroughly understand how repository secrets are used in PRs before approving a PR like this, and if they're not available, do the tests run or will contributors be asked to chase false positives?

rewriting

[tgummerer]

I'd like to thoroughly understand how repository secrets are used in PRs before approving a PR like this, and if they're not available, do the tests run or will contributors be asked to chase false positives?

Yeah we will definitely need to do some more cleaning up of tests before being able to do that. I don't think the tests currently run well that way, but we should clean that up anyway.

As for how secrets are used currently, I believe there's two parts to that:

• Secrets to access the pulumi cloud. I think we might want to change some of these tests to only use the local backend, while keeping some that test the actual interface with the Pulumi cloud. These are mostly integration tests, that I think would be okay to be skipped for a quick turnaround, but would ideally be run eventually.
• Access keys to cloud backends (AWS, S3, Azure), to test our blob backends. These are not super relevant for most changes, but are very important tests for the few changes that do require these backends. There's probably very little loss if those tests are not being run on most external PRs.

And I do believe we should still have a slash command to actually run all the tests.

To make sure external contributors don't have problems with flaky tests here we might want to set up a cron-job that runs without secrets and alerts us when it's broken. Note that we sort of already have this problem sometimes in that the workflow we're running can bitrot if we're not careful to add new secrets there, so this is something we would ideally have either way.

[AaronFriel]

@tgummerer I do like the idea of the workflow / cron that somehow runs without secrets, as if it fails that will create a P1 with our workflow automation.