New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow the regular CI pipeline to be run on external PRs #16083
base: master
Are you sure you want to change the base?
Conversation
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.
Changelog[uncommitted] (2024-04-29) |
Docs for how this GitHub feature works: https://docs.github.com/en/actions/managing-workflow-runs/approving-workflow-runs-from-public-forks |
Blocked on internal discussion |
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:
The latter issue complicates things a little bit, since even with approvals users don't have access to secrets. On the other hand the So for this repo I would propose the following:
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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:
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. |
@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. |
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.