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

design proposal: series of mergeability rules in separate files #77

Closed
jeffkaufman opened this Issue Jan 26, 2019 · 15 comments

Comments

Projects
None yet
5 participants
@jeffkaufman
Copy link
Owner

jeffkaufman commented Jan 26, 2019

In #76 we've been talking about how to structure the validation steps. Before one of us goes and puts the time into writing something robust, though, we should figure out what this should look like.

My overall sense of how this should work is something like:

  • validate.py collects information that rules need, then triggers the rules
  • each rule is in its own file
  • rules say something like "allow merging if ..." or "reject merging if ..."
  • rules run in order, and the first one to trigger wins
  • if an allow-merging rule raises an exception then it's as if the rule didn't exist
  • if a block-merging rule raises an exception then merging is blocked

Right now our structure looks like:

  1. allow merging if points transfer
  2. block merging if there are rejections
  3. block merging if there aren't enough approvals
  4. block merging if the pr is too recent and not unanimously approved
  5. allow merging

This could be implemented as something like:

$ ls rules/
0.1-allow-points-transfer.py
0.2-block-vetos.py
0.3-block-approvals.py
0.4-block-recent.py

The number before the dash is floating point, and determines what order the rules run in. All numbers must be > 0 and < 1.

The contents of the files look like:

def allow_if(pr):
   # Raise an Exception for not allowing and processing continues, otherwise allow and processing ends.

or

def reject_if(pr):
   # Raise an Exception for rejecting and processing ends, otherwise processing continues.

Some examples, none of which I've run or tested:

  • 0.1-points-transfer.py:
def allow_if(pr):
  # If a PR only moves points around by the creation of new bonus files, has
  # been approved by every player losing points, reduces the total number of
  # points, allow it.
  #
  # Returns to indicate yes, raises an exception to indicate no.
  #
  # Having a PR merged gives you a point (#33), so a PR like:
  #
  #  - me:  -2 points
  #  - you: +1 point
  #
  # is effectively:
  #
  #  - me:  -1 point
  #  - you: +1 point

  print('\nConsidering whether this can be merged as a points transfer:')

  diff = pr.diff()
  if diff.modified_files or diff.removed_files:
    raise Exception('All file changes must be additions')

  total_points_change = 0

  for added_file in diff.added_files:
    s_players, points_user, s_bonuses, bonus_name =  added_file.path.split('/')
    if s_players != 'players' or s_bonuses != 'bonuses':
      raise Exception('Added file %s is not a bonus file')

    (diff_invocation_line, file_mode_line, _, removed_file_line,
     added_file_line, patch_location_line, file_delta_line,
     empty_line) = str(added_file).split('\n')

    if diff_invocation_line != 'diff --git a/%s b/%s' % (
        added_file.path, added_file.path):
      raise Exception('Unexpected diff invocation: %s' % diff_invocation_line)

    if file_mode_line != 'new file mode 100644':
      raise Exception('File added with incorrect mode: %s' % file_mode_line)

    if removed_file_line != '--- /dev/null':
      raise Exception(
        'Diff format makes no sense: added files should say they are from /dev/null')

    if added_file_line != '+++ b/%s' % added_file.path:
      raise Exception('Something wrong with file adding line: file is '
                      '%s but got %s' % (added_file.path, added_file_line))

    if patch_location_line != '@@ -0,0 +1,1 @@':
      raise Exception('Patch location makes no sense: %s' %
                      patch_location_line)

    if empty_line:
      raise Exception('Last line should be empty')

    if file_delta_line.startswith('+'):
      actual_file_delta = file_delta_line[1:]
    else:
      raise Exception('File delta missing initial + for addition: %s' %
                      file_delta_line)

    # If this isn't an int, then it raises and the PR isn't mergeable
    points_change = int(actual_file_delta)

    if points_change < 0:
      if points_user not in pr.approvals:
        raise Exception('Taking %s points from %s requires their approval.' % (
            abs(points_change), points_user))

    total_points_change += points_change

  if total_points_change >= 0:
    raise Exception('points change PRs must on net remove points')

  # Returning without an exception is how we indicate success.
  print('  yes')
  • 0.2-block-vetos.py:
def reject_if(pr):
  if pr.rejections:
    raise Exception('Rejected by: %s' % (' '.join(pr.rejections)))
  • 0.3-block-approvals.py:
def reject_if(pr):
  required_approvals = math.ceil(len(util.users()) * 2 / 3)

  # Allow three days to go by with no commits, but if longer happens then start
  # lowering the threshold for allowing a commit.
  approvals_to_skip = util.days_since_last_commit() - 3
  if approvals_to_skip > 0:
    print("Skipping up to %s approvals, because it's been %s days"
          " since the last commit." % (approvals_to_skip,
                                       util.days_since_last_commit()))
    required_approvals -= approvals_to_skip

  if len(pr.approvals) < required_approvals:
    raise Exception('Insufficient approval: got %s out of %s required approvals' % (len(pr.approvals), required_approvals))
  • 0.4-block-recent.py:
def reject_if(pr):
  # Don't allow PRs to be merged the day they're created unless they pass unanimously
  if (len(pr.approvals) < len(util.users())) and (pr.days_since_created() < 1):
    raise Exception('PR created within last 24 hours does not have unanimous approval.')
@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Jan 26, 2019

@tnelling @sligocki @pavellishin @vesche @shaunagm looking for design review

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Jan 26, 2019

This is exactly what I had in mind, except with more thought put into it. Not only does it provide clarity, but it should allow us to do more creative things with what kinds of PR require what level of approval. For instance, PRs that don't touch validate.py or the rules or players folders could be easier to pass. PRs that touch validate.py could be harder to pass. PRs adding files to rules could auto-fail without the formatting you mentioned. The details are unimportant - I like what the structure potentially enables.

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Jan 27, 2019

@tnelling

The details are unimportant

While the details aren't that important, I'd like to try to get rough agreement on details before I go implement something, to reduce the risk that I need to make major changes after sending something out. (But maybe you're saying this is more or less fine?)

@shaunagm

This comment has been minimized.

Copy link
Contributor

shaunagm commented Jan 28, 2019

I've only skimmed this, but I like the overall structure. My only question is what "rules run in order, and the first one to trigger wins" means.

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Jan 28, 2019

@shaunagm good question, and that's core to the design so we should definitely be clear on it!

Processing runs through an ordered series of accept_if and reject_if filters. In pseudo code:

def should_allow_merging(pr):
  for filter in filters:
    if is accept filter and filter accepts pr:
      return True
    else if is reject filter and filter rejects pr:
      return False
  return True

Each filter can either make a decision (definitely accept this, definitely reject this) or allow processing to continue. For example, imagine a PR that's been vetoed. Processing would look like:

  1. The "is this a points transfer PR" filter is an accept filter, and in this case says "I don't apply". Processing continues with the next filter in order.

  2. The "block vetoes" filter is a reject filter, and in this case says "I do apply". Processing stops and the PR is rejected.

If the "block vetoes" filter had said "I don't apply" then we would have continued on to the "do we have enough approvals" reject filter.

@shaunagm

This comment has been minimized.

Copy link
Contributor

shaunagm commented Jan 28, 2019

Okay, that makes sense and is what I expected from the rest of the description. I wouldn't describe that as "first one to trigger wins" but that's a nitpick.

@sligocki

This comment has been minimized.

Copy link

sligocki commented Jan 29, 2019

This is great. One proposal: Instead of having two types of filters, what if any filter could return one of 3 values:

  • APPROVE - This PR is immediately approved without evaluating future rules
  • REJECT - This PR is immediately rejected without evaluating future rules
  • NO_JUDGEMENT - Move on to next rule

I think that would be clearer than having different classes of filter and having to remember what the API for each was.

@sligocki

This comment has been minimized.

Copy link

sligocki commented Jan 29, 2019

I guess a counter-argument to my own comment might be: Allow filters are more dangerous than block filters, so forcing those words into the file name makes it a little less likely someone will sneakily add an allow filter (vs. returning APPROVE in an unlabeled filter, which might be easier to hide ...) Hm.

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Jan 29, 2019

As I understand it, the filters each return a boolean, to be interpreted as follows:

Allow filter: true means pass validation, false means continue to next rule
Block filter: true means continue to next rule, false means fail validation

What I like about this system over the ternary one, aside from not needing to define custom types or use strings, is that it makes it very clear what sort of rule you're writing. We won't end up with rules that always result in a final verdict on validation (either returning APPROVE or REJECT), thus short-circuiting any subsequent rules.

@tnelling

This comment has been minimized.

Copy link
Collaborator

tnelling commented Jan 29, 2019

@tnelling

The details are unimportant

While the details aren't that important, I'd like to try to get rough agreement on details before I go implement something, to reduce the risk that I need to make major changes after sending something out. (But maybe you're saying this is more or less fine?)

Yeah, I was trying to say that the details I myself was going over weren't super important.

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Jan 29, 2019

@tnelling

As I understand it, the filters each return a boolean

I was actually proposing the filter API uses exceptions. Exceptions have to be part of the contract, because people are going to write code that generates runtime exceptions sometimes and we need to do something sensible when it does. For an allow filter, I think it's pretty clear that an exception should mean "don't allow this, move on to the next filter", the same as no judgement. For a reject filter I think an exception should mean "reject this", because otherwise there's a risk of a faulty reject filter letting things through. (We should have a super-simple unanimous allow filter at the beginning of the filter chain, to let us get unstuck if we accidentally lock ourselves out with a broken reject filter.) Once exceptions have these meanings, we can keep things simple by having them be the only way filters return their results.

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Jan 29, 2019

Filters should also receive a copy of the PullRequest object -- we don't want one filter fiddling with it and confusing a later one.

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Jan 29, 2019

There could also be a dictionary of freely modifiable key-value pairs that filters can use to talk to each other with.

@jeffkaufman

This comment has been minimized.

Copy link
Owner Author

jeffkaufman commented Jan 29, 2019

This is starting to sound a lot like the filter chain in a webserver...

@pavellishin pavellishin referenced this issue Jan 29, 2019

Closed

dchudz won #83

@pavellishin

This comment has been minimized.

Copy link
Collaborator

pavellishin commented Jan 29, 2019

I'm very interested in this, and want to read over it when I get a chance :/

This was referenced Jan 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment