Moves to hashtag #mergeongreen

ashfurrow commented 5 years ago

Peril reads comments and PR reviews for the phrase "merge on green" to mark a PR as needing to be merged when CI is green. The problem is, there's nothing about the phrase "merge on green" that indicates that you're directing Peril, instead of the engineer.

This PR changes the words Peril search for to a hashtag: #mergeongreen. This hashtag format mirrors the other use of Peril looking for text:

https://github.com/artsy/peril-settings/blob/868d4ba97dbe3f268c84135abbb707b537b95a01/org/allPRs.ts#L70-L73

And we use a similar convention with Danger in a few repos, for example:

https://github.com/artsy/emission/blob/3ab51744d52cf7e4410d36e2f46c3a6019bb0a93/dangerfile.ts#L18-L20

This PR is marked ⚠️ DO NOT MERGE YET ⚠️ because, even though it's not important enough of a change to make an RFC, it is important enough of a change to communicate clearly. To that end, we'll plan on the following:

I'll mention in Slack today that this change is upcoming.
We'll mention it again during Monday's standup.
I'll mention it again in Slack after Monday's standup.
Then we'll merge this PR (it will take effect immediately).

Let me know if I can clarify any of this.

peril-staging[bot] commented 5 years ago

	Fails
:no_entry_sign:	Danger failed to run `peril-downloaded-artsy/peril-settings@org/addReviewer.ts`.

Error TypeError

Cannot read property 'requested_reviewer' of undefined
TypeError: Cannot read property 'requested_reviewer' of undefined
    at Object.exports.rfc177_2 (peril-downloaded-artsy/peril-settings@org/addReviewer.ts:7:28)
    at Object.<anonymous> (/opt/out/node_modules/danger/distribution/runner/runners/inline.js:147:60)
    at step (/opt/out/node_modules/danger/distribution/runner/runners/inline.js:32:23)
    at Object.next (/opt/out/node_modules/danger/distribution/runner/runners/inline.js:13:53)
    at /opt/out/node_modules/danger/distribution/runner/runners/inline.js:7:71
    at new Promise (<anonymous>)
    at __awaiter (/opt/out/node_modules/danger/distribution/runner/runners/inline.js:3:12)
    at Object.exports.runDangerfileEnvironment (/opt/out/node_modules/danger/distribution/runner/runners/inline.js:105:136)
    at runDangerPR (/opt/out/runner/run.js:137:44)
    at <anonymous>
    at process._tickDomainCallback (internal/process/next_tick.js:228:7)

Dangerfile

 2| import { PRReviewMetadata } from "../tasks/prReviewReminder"
 3| import { PullRequest } from "github-webhook-event-types"
 4| import { PullRequestPullRequestPullRequestUser as User } from "github-webhook-event-types/source/PullRequest"
 5| 
 6| // This interface gets around the fact that github-webhook-event-types doesn't include a requested_reviewer field
------------------------------^
 7| // on pull_request.review_requested event types (requested_reviewer only exists on certain events).
 8| // See the GitHub API documentation here: https://developer.github.com/v3/activity/events/types/#webhook-payload-example-28
 9| export interface RequestedReview extends PullRequest {
10|   requested_reviewer: User

Generated by :no_entry_sign: dangerJS against 59fe1cb28ca3e844e29cd1a9bc5850b33ae5b915

ashfurrow commented 5 years ago

This failure is a known-issue ☝️ Once we migrate to our own Peril hosting, it should become easy to debug and fix it, then roll out to the whole org 🎉

mzikherman commented 5 years ago

Minor point I've wondered about this functionality, now seems to be a reasonable time to bring it up (since there's a PR!). Sorry that this work sparked the thought @ashfurrow

There is a Peril rule and functionality that will merge a PR, when the label 'Merge on Green' is applied, and the tests are green.

There is a separate Peril rule, that will apply the 'Merge on Green' label, when certain text appears (that's the rule being modified/discussed here).

Am I alone in thinking that's a bit over-engineered, and that we don't actually need the second rule? If a developer wants Peril to merge on green, they can add that label themselves (which seems preferable than requiring special text in the PR description or comments).

ashfurrow commented 5 years ago

@mzikherman This is a good question – you're right that there are two Peril rules here. I disagree that it's over-engineered because: engineers are lazy. Especially me! If I had to, after submitting a PR review, go and add the M-O-G label, then that's extra work that I'd rather have a computer do for me. Especially since some repos have many labels, and this rule also handles creating the M-O-G label on repos that don't yet have it.

I don't think M-O-G would get used as often if it didn't have that second rule.

zephraph commented 5 years ago

It's a bit of a choose your own adventure style auto merging now 😄. Taking a convention that exists, removing it, and asking people to follow a new convention is more work than allowing folks to pick between different existing options. Especially when there's already familiarity around those conventions.

I like how this makes it more explicit! 👍

mzikherman commented 5 years ago

@ashfurrow very fair points, especially the one about creating the label for you!

ashfurrow commented 5 years ago

Ha, should have updated the PR title before merging 🙈

artsy / peril-settings

Moves to hashtag #mergeongreen #129

Error TypeError

Dangerfile