artsy/README

[RFC] Scope Peril ticket automation to require brackets around ticket numbers

ashfurrow opened this issue · 10 comments

ashfurrow commented

Proposal:

Modify the regular expression used by Peril, which looks through our commits and pull request descriptions for Jira ticket numbers. This is used to automatically move tickets through the Jira board (eg: when the PR is merged, move it to the "Merged" Jira column).

Reasoning

The automation, while useful for engineers and PDDE generally, as well as stakeholders, leads to false positives. For example, this commit included a link to a related, future ticket that I'd just opened. Peril moved the ticket to "Merged" when the commit's PR was merged, even though the ticket wasn't completed, but only referenced in the commit.

This happens to me quite a bit, and I wonder if it happens to others. I'd like to scope the lookup regex to require either (brackets) or [square brackets] to surround ticket numbers in commits, PR titles, and PR bodies.

I don't expect this to be a controversial change, but wanted to make sure I heard from others before potentially breaking anyone's workflows.

Exceptions:

None that I can think of.

How is this RFC resolved?

A PR to peril-settings:

- const jiraTicketRegex = /\d+-[A-Z]+(?!-?[a-zA-Z]{1,10})/g
+ const jiraTicketRegex = /[\[\(]\d+-[A-Z]+(?!-?[a-zA-Z]{1,10})[\]\)]/g
kierangillen commented

This wont have any effect on linking directly in the body of the PR with the url of the jira ticket, correct?

E.g.

ashfurrow commented

It would. Linking to the ticket would no longer be enough to have it get picked up by Peril (you would need to include just the ticket number in brackets).

anandaroop commented

The proposal makes sense to me, but I'm having a little trouble understanding how the matching is supposed to work — see here

Maybe a few documented examples of what kinds of ticket strings would and wouldn't work might make the intended behavior a little clearer?

ashfurrow commented

@anandaroop really great point! I looked into this further, reading the code and its link to the source of its regex. It's complicated by the fact that the regex requires the string to be reversed before matched against, due to alleged limitations in the JavaScript regex engine. Yikes.

All that said, here's what I'm expecting to work/not work:

This would require removing the jiraURLRegex check from the code I linked to above. I've also modified the RFC to link to the same code.

anandaroop commented

Ha, sounds tricky 😅. Thanks for the examples!

erikdstock commented

What about when naming a ticket in a markdown link like this?

[AUCT-xxx](https://artsyproduct.atlassian.net/browse/AUCT-xxx).

I would not necessarily want this to work unless i was doing [[AUCT-xxx]]

ashfurrow commented

@erikdstock that's a good point, hmm. I think we can be smart about the regex to avoid this (ie: make sure it isn't a markdown link). Something like:

const jiraTicketRegex = /[^\)][\[\(]\d+-[A-Z]+(?!-?[a-zA-Z]{1,10})[\]\)]/g

The [^\(] at the beginning (which is the end of the reversed string, see above) specifies that the ticket number shouldn't be in a markdown link. I will make sure to include updated unit tests for this behaviour 👍

ashfurrow commented

Resolution

We decided to do it.

Level of Support

2: Positive feedback.

Additional Context:

We learned more about Peril's use of regular expressions to look for ticket numbers. We also will make sure to ignore any that are surrounded by [square brackets] because they are Markdown links and shouldn't count.

Next Steps

I'll open a pull request. On Monday's standup, I'll include this in the requests-for-pairing section.

Exceptions

None.

jonallured commented

Sorry, late to the party and I know there's already been work done here, but wanted to clarify something: does this impact automation based on the ticket number in the tite of the PR? I read and re-read this stuff but only see the PR description and commit messages referenced so maybe I'm missing something, but my process has been to mention the Jira ticket in the title of the PR, like this:

https://github.com/artsy/volt/pull/4057 - GALL-2443: Use schema validation for publish button on artwork detail page
https://github.com/artsy/volt/pull/4055 - GALL-2465: Destroy all that remains of validate commerce
https://github.com/artsy/volt/pull/4051 - GALL-2440: More strict validation on published works

So I guess my questions are: do I have to start adding brackets to these PR titles? Is mentioning the Jira ticket in the title enough to trigger the integration that moves tickets based on them being merged or do I have to mention it in the PR description?

ashfurrow commented

@jonallured that's a good question. The code has only ever looked in PR bodies and commit messages and not titles (which surprised me, to be honest). https://github.com/artsy/volt/pull/4057 only triggered the automation because the merge commit had the ticket in its commit message. So all you need to do to continue working with the workflow is add brackets to the title. Let me know what I can clarify.

  • 👍1