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

Add authorize job which requires externally created PRs to be approve… #883

Merged
merged 3 commits into from
Jan 15, 2025

Conversation

kylewuolle
Copy link
Contributor

This PR enables the concept of approving externally created PRs for running e2e tests. It is based on a blog post : https://datachain.ai/blog/testing-external-contributions-using-github-actions-secrets

The setup involved here is first we create two environments: external and internal. The external environment will have a deployment protection rule enabled which will require a review from anyone on the team. This PR introduces a job called "authorize" which only runs after approval if the PR originates from a fork. Then, the e2e test jobs are marked as needing the authorize job to complete prior to running. In this way users on the team can still run e2e tests from either forked or unforked PRs since we can approve the job.

Here is an example of the protection in action:
vokoscreen-2025-01-10_09-52-32

Here's an example of a PR which is not forked:
vokoscreen-2025-01-10_10-06-37

For forked PRs from team members the flow will be the same as the first example but we can self approve the job run.

Resolves https://github.com/k0rdent/2a-internal/issues/17

…d while allowing internal team members to run tests without approval
@zerospiel
Copy link
Contributor

A quick clarification: so the new job authorize is valid only for the e2e-related jobs, and the flow is still the same, like to set the labels? However, only an authorized team member could set the label, right? Could you please elaborate on this? In other case, the approach looks ok to me

@DinaBelova
Copy link
Collaborator

I think keeping labels is okay (if that is the case) as even for PRs produced by main dev team e2e tests run (and which exact one) may be different depending on the commit. So I think having labels as the instrument to specify what tests exactly to run seems ok.

@kylewuolle please confirm the labels approach was untouched ^^.

@kylewuolle
Copy link
Contributor Author

A quick clarification: so the new job authorize is valid only for the e2e-related jobs, and the flow is still the same, like to set the labels? However, only an authorized team member could set the label, right? Could you please elaborate on this? In other case, the approach looks ok to me

Yes that's right so the label is still required in order for the e2e tests to run. So the flow is basically the same other than if the e2e test label is set the authorize job must be approved by a team member. Team members can self approve their own PRs and external users' PRs will need to be approved.

@kylewuolle kylewuolle requested a review from eromanova as a code owner January 13, 2025 16:55
Copy link
Contributor

@eromanova eromanova left a comment

Choose a reason for hiding this comment

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

@kylewuolle I like the approach, but could you please clarify something? So as I understand:
• For PRs created from forked repositories, additional approval from members of the k0rdent team is required
• For unforked PRs (pushed directly to the original kcm project), no additional protection is applied since only team members have permission to push directly to k0rdent.
Am I correct in understanding this? Specifically, I’m curious whether the protection will function correctly for PRs created directly in the repository.

@kylewuolle
Copy link
Contributor Author

@kylewuolle I like the approach, but could you please clarify something? So as I understand: • For PRs created from forked repositories, additional approval from members of the k0rdent team is required • For unforked PRs (pushed directly to the original kcm project), no additional protection is applied since only team members have permission to push directly to k0rdent. Am I correct in understanding this? Specifically, I’m curious whether the protection will function correctly for PRs created directly in the repository.

Yes that's right. So unforked PRs will not require approval and forked ones will. Since every team member is able to approve PRs for e2e test runs (including their own) this will not cause issues where team members will need to get someone else to approve a test run. Hopefully that makes sense.

@kylewuolle kylewuolle requested a review from eromanova January 15, 2025 18:44
@DinaBelova
Copy link
Collaborator

DinaBelova commented Jan 15, 2025

please do not merge until we'll do the needed changes in the repo settings (envs setup)
upd: added the external environment with @k0rdent/2a-dev team as required reviewer

Copy link
Collaborator

@a13x5 a13x5 left a comment

Choose a reason for hiding this comment

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

I think that we can try that approach. LGTM

@a13x5 a13x5 merged commit 607bb46 into k0rdent:main Jan 15, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants