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

ci: requiring two org members to approve for PR merge; preventing in org sock puppet accounts #9671

Conversation

Darien-Lin
Copy link
Contributor

@Darien-Lin Darien-Lin commented Jan 17, 2025

Description

This adds multi-approver workflow following this: https://github.com/abcxyz/pkg?tab=readme-ov-file#multi-approversyml.
This prevents in-org sock puppet accounts.

@Darien-Lin Darien-Lin requested a review from a team as a code owner January 17, 2025 19:19
Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

Hello Darien-Lin and reviewers,

I'm Gemini, and I'm currently reviewing this pull request. In the meantime, I'm providing a summary of its content to help everyone quickly understand the changes and intent.

This pull request, titled "ci: requiring two org members to approve for PR merge; preventing in org sock puppet accounts", aims to enhance the CI process by implementing a multi-approver workflow. This is intended to prevent the use of sock puppet accounts within the organization.

Here's a breakdown of the changes:

  • .github/workflows/members.json: This file has been added and contains a JSON array listing GitHub usernames of organization members who are authorized to approve pull requests. The list is quite extensive, containing 80+ members (lines 1-233).
  • .github/workflows/multi-approvers.yml: This workflow file (lines 1-31) has been added to define the multi-approver process. It uses a workflow from abcxyz/pkg and specifies the path to the members.json file.

The implementation leverages a multi-approver YAML configuration (linked in the PR description) to enforce the requirement of two approvals before a pull request can be merged. This should significantly improve the security and quality control of the merge process.

I'll have a more detailed review shortly.


And now, a little haiku to lighten the mood:

Two heads are better,
Than one, to check the code flow,
Secure the merge path.

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

This pull request implements a multi-approver workflow to enhance security and prevent sock puppet accounts. The approach of using a reusable workflow and a separate JSON file for organization members is well-structured. However, the current implementation in members.json lists all organization members, which might not be the intended behavior for a multi-approver workflow. Typically, a smaller subset of trusted members would be designated as approvers.

Key Recommendations before Approval:

  • Clarify the purpose of members.json: Is this intended to list all organization members or only designated approvers? If it's the latter, consider renaming the file to approvers.json or similar for clarity.
  • Refine the member list: If members.json is for approvers, reduce the list to the intended individuals. If it's for all members, consider how this interacts with the multi-approver workflow. Does every member have approval authority?
  • Document the workflow: Add documentation explaining the multi-approver workflow, including how to modify the approver list and any other relevant details. This could be in the repository's README or a separate CONTRIBUTING.md file.
  • Consider alternative approaches: Explore GitHub's built-in branch protection rules for requiring multiple approvals. This might simplify the workflow and eliminate the need for custom actions.

@@ -0,0 +1,233 @@
[
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wait, this is the approved workflow? We're supposed to manually maintain this file?

Is there a way to just add skaffold-team to this file? I don't even know who most of these people are

Copy link
Collaborator

Choose a reason for hiding this comment

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

(And skaffold-team has the advantage of being automatically synced to our internal systems)

@Darien-Lin Darien-Lin force-pushed the skaffold-multi-approvers branch from 45f8a13 to 958bf3b Compare January 17, 2025 19:24
@Darien-Lin Darien-Lin closed this Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants