Skip to content

Add merge rules engine for validating PRs before landing#317

Open
seemethere wants to merge 2 commits intomasterfrom
gh/seemethere/1/head
Open

Add merge rules engine for validating PRs before landing#317
seemethere wants to merge 2 commits intomasterfrom
gh/seemethere/1/head

Conversation

@seemethere
Copy link
Collaborator

Introduces a new merge_rules module that provides:

  • MergeRule dataclass for rule configuration (patterns, approvers, checks)
  • MergeRulesLoader to load rules from .github/merge_rules.yaml
  • MergeValidator to validate PRs against rules including:
    • File pattern matching (fnmatch/glob)
    • Approver validation with team expansion (org/team-slug)
    • CI check status validation
  • ValidationResult and MergeValidationError for error handling

Also adds new GitHub API methods to GitHubEndpoint:

  • get_pr_reviews, get_pr_files, get_check_runs
  • get_team_members, get_file_contents, post_issue_comment

Adds pyyaml dependency for YAML parsing.

[ghstack-poisoned]
[ghstack-poisoned]
return github_state(info).repositories[self._repository]


@dataclass
Copy link
Owner

Choose a reason for hiding this comment

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

Recommend adding tests for the fakes

# Merge rules related fields
files: List[str] = dataclasses.field(default_factory=list)
reviews: List[PullRequestReview] = dataclasses.field(default_factory=list)
check_runs: List[CheckRun] = dataclasses.field(default_factory=list)
Copy link
Owner

Choose a reason for hiding this comment

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

There doesn't seem to be any API for populating these


This module provides functionality to load, parse, and validate merge rules
that control when PRs can be landed. Rules specify required approvers and
CI checks based on file patterns.
Copy link
Owner

Choose a reason for hiding this comment

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

Where is the spec for merge rules YAML?

logging.debug("No merge_rules.yaml found in repository")
return []
except Exception as e:
logging.warning(f"Failed to load merge rules: {e}")
Copy link
Owner

Choose a reason for hiding this comment

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

logging.exception better here


This module provides functionality to load, parse, and validate merge rules
that control when PRs can be landed. Rules specify required approvers and
CI checks based on file patterns.
Copy link
Owner

Choose a reason for hiding this comment

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

Need to explain that this is intended to be used by ghstack in a privileged context, not by the user themselves


def get_pr_approvers(self, pr_number: int) -> Set[str]:
"""Get set of users who have approved the PR."""
reviews = self.github.get_pr_reviews(self.owner, self.repo, pr_number)
Copy link
Owner

Choose a reason for hiding this comment

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

Do I have to worry about pagination for this REST query

Copy link
Owner

Choose a reason for hiding this comment

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

I think you do.

if not head_sha:
return {}

check_runs = self.github.get_check_runs(self.owner, self.repo, head_sha)
Copy link
Owner

Choose a reason for hiding this comment

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

Also pagination risk

Copy link
Owner

@ezyang ezyang left a comment

Choose a reason for hiding this comment

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

Pagination not done correctly

except Exception as e:
logging.warning(f"Failed to expand team {team_ref}: {e}")
# Return as single user if team expansion fails
return {team_ref}
Copy link
Owner

Choose a reason for hiding this comment

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

Suppress exception here seems sloppy


try:
org, team_slug = team_ref.split("/", 1)
members = self.github.get_team_members(org, team_slug)
Copy link
Owner

Choose a reason for hiding this comment

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

Worried about pagination on this one too

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.

2 participants