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 repository predicate #461

Merged
merged 2 commits into from
Aug 30, 2022

Conversation

devinburnette
Copy link
Contributor

Sometimes we find ourselves wanting to make exceptions for certain policies, but don't want to have individual policy files in the repos that are exceptional. This PR would add a new repository predicate that would perform simple matching on the repository and from that information decide whether or not a rule should be evaluated. I could see this being especially useful for organizations that want to use a centralized policy to govern all repos.

Another area where an addition like this brings value is when setting up policy bot as a github application, you're given an option to install on all repositories or on select repositories. If an organization has 100s of repositories it may be difficult to configure policy bot on all but some. On the other hand, if you select the all repos option, you end up with pending status checks on PRs that may cause confusion. This configuration would allow you to configure automatic approval for certain repos allowing orgs to put their exceptions "in code" and select the all repos option which makes sure that most new repos will receive the benefits of policy bot automatically without any additional configuration.

policy:
  approval:
    - or:
      - normal review
      - docs review

approval_rules:
- name: normal review
   if:
     repository:
       not_matches:
        - "palantir/.*docs"
   requires:
     count: 1
- name: docs review
  if:
    repository:
      matches:
       - "palantir/.*docs"
  requires:
    count: 0

Let me know what you think of this idea, and if you think there's a better way of accomplishing the same. And as always, thanks for maintaining an awesome tool!

Copy link
Member

@bluekeyes bluekeyes left a comment

Choose a reason for hiding this comment

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

Thanks for proposing this. Supporting new predicates, especially ones that don't require loading new data, is cheap so I'm happy to add this repository predicate.

That said, I'm suspicious that in the long-term trying to carve out repository-specific exemptions in a single policy is going to be unmaintainable. How we've tried to handle this so far is by minimizing policy differences (e.g. by using repository permission levels instead of team references) and putting all remote policies in a single repository so they are easy to review. It's mostly worked so far, but there are definitely still rough edges.

We've also had several requests for methods to merge or combine policies so that a repository can inherit a default policy, but then add some additional rules. We haven't thought of a way to achieve this yet that is understandable and prevents undermining the security guarantees of inherited policies.

func (pred Repository) Evaluate(ctx context.Context, prctx pull.Context) (*common.PredicateResult, error) {
owner := prctx.RepositoryOwner()
repo := prctx.RepositoryName()
repoFullName := strings.Join([]string{owner, repo}, "/")
Copy link
Member

Choose a reason for hiding this comment

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

nit: for two strings, I'd probably just concat them: owner + "/" + repo

README.md Outdated
Comment on lines 251 to 253
# "repository" is satisfied if the pull request repository matches any one of the
# patterns within the "matches" list, and does not match all of the patterns
# within the "not_matches" list.
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this behavior is what is actually implemented. Based on the code and tests, I believe the matches list always wins: if the repository matches, the predicate is satisfied. Looking at the existing title predicate, I see its behavior also doesn't match what the docs say.

Unless you want to change the actual behavior, let's update this comment to be:

# "repository" is satisfied if the pull request repository matches any one of the
# patterns within the "matches" lis or does not match all of the patterns
# within the "not_matches" list.

If you don't mind also updating the title docs, I'd appreciate it.

@bluekeyes bluekeyes merged commit 38b8a99 into palantir:develop Aug 30, 2022
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