Skip to content
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

Use native GitHub approvals for community PRs #905

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Conversation

blampe
Copy link
Contributor

@blampe blampe commented Apr 29, 2024

Edit: forked PRs still don't get secrets, and we'll likely need to work around that with pull_request_target.

Inspired by pulumi/pulumi#16083.

GitHub natively allows maintainers to manually kick off tests on PRs from community members. It's straightforward and doesn't hide test results behind a URL.

This PR removes all the logic around /run-acceptance-tests in favor of GitHub's native functionality.

Caveat: GitHub doesn't currently provide an API for configuring fork approval settings (integrations/terraform-provider-github#2108), and the default is "Require approval for first-time contributors" whereas the current workflow is equivalent to "Require approval for all outside collaborators".

I've changed the pulumi org's default permission to always require approval for outside PRs, and this seems to have had the intended effect.

@@ -66,7 +49,6 @@ jobs:
uses: actions/checkout@v4
with:
lfs: true
ref: ${{ env.PR_COMMIT_SHA }}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was being appended as part of addDispatchConditional. In theory it's not needed -- GitHub will checkout a merge commit of the PR by default, which is good. We got rid of this in some other actions as part of #687 which makes me more confident it's safe to remove.

@blampe blampe requested a review from a team April 29, 2024 20:33
@blampe blampe requested a review from tgummerer April 29, 2024 21:28
Copy link
Member

@danielrbradley danielrbradley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great simplification!

By changing the org-level setting does this mean we don't need to set it to always require approvals on a per-repo basis?

@blampe
Copy link
Contributor Author

blampe commented Apr 29, 2024

By changing the org-level setting does this mean we don't need to set it to always require approvals on a per-repo basis?

I think so, yes. pulumi/kubernetes was configured for "first-time contributors" before I tweaked the org-level setting, but afterwards it showed "all outside collaborators". Seems to be the case with all the other repos I spot-checked as well.

@blampe blampe marked this pull request as draft April 30, 2024 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants