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

Rework rule format to distinguish between trigger and actions #213

Open
cyamonide opened this issue Oct 17, 2020 · 3 comments
Open

Rework rule format to distinguish between trigger and actions #213

cyamonide opened this issue Oct 17, 2020 · 3 comments
Labels

Comments

@cyamonide
Copy link
Collaborator

cyamonide commented Oct 17, 2020

In Phabricator's Herald, conditions and actions are made distinct:
image

use-herald-action's rules should take on a similar interface.

Motivation

Currently, rules exist in the form of a flat JSON object which makes it difficult to understand the cause and effect of a particular UHA rule at a glance.

An example

Current rule

{
  "name": "Add my-awesome-team as a reviewer for relevant changes.",
  "includes": [
    "src/alpha",
    "src/bravo",
    "src/charlie"
  ],
  "action": "review",
  "teams": ["my-awesome-team"]
}

To the uninitiated, the teams attribute above is ambiguous, and could be a trigger condition. Does it mean to add my-awesome-team as a reviewer, or is it a rule that triggers when a member of my-awesome-team opens the PR?

After inspecting the rule, it's possible to deduce the former. However, it shouldn't take EMs and new developers a few minutes to figure out what a rule does.

Reworked rule

{
  "name": "Add my-awesome-team as a reviewer for relevant changes.",
  "triggers": {
    "includes": [
      "src/alpha",
      "src/bravo",
      "src/charlie"
    ],
  },
  "actions": [
    {
      "action": "review",
      "teams": ["my-awesome-team"]
    }
  ]
}

The new format (above) distinguishes triggers and actions (cause and effect) much better.

Extensibility

Instead of having users determine a JSONPath expression for triggering a rule when, say, a specific user has opened a PR, it would be nice if a rule could accept a list of users on which to trigger the rule.

However, there is already a users field accepted by our current rule format, and it is used in the action (not the trigger) of the rule.

@cyamonide cyamonide added the 3.0 label Oct 17, 2020
@ricky-wong
Copy link

Maybe we could have multiple triggers and specify ALL or ONE?

Also, if we could have multiple actions (simply an array of objects?), we can save people from the rule-duplication that's currently happening.

@gagoar
Copy link
Owner

gagoar commented Oct 18, 2020

Interesting. I think it makes things more complex to write, but way more clear as well.

I like the idea of dividing triggers altho, we are gonna need a more comprehensive idea about how to structure it to avoid issues.

re concept: We have Phabricator like functionality, but I don't believe we should duplicate Phabricator's interface, meaning is not a recipe for success (necessarily). I like the atomic approach we have set so far. altho some duplication exists, we can debug and share more simply what's going on in a rule. Simplicity keeps being key. Rules are meant to be written once, and read many times, also hopefully re-used.

@gagoar
Copy link
Owner

gagoar commented Dec 28, 2020

@cyamonide do you mind investigate how our current rules will look like with this new format? I would hate to do this work without anticipating issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants