artsy / peril-settings Goto Github PK

This is currently a Peril rule, but wondering if it might make more sense as a Horizon function.

One downside is that it seems as though Peril comments disappear after the PR is merged, which definitely is not ideal.

While this type of 'nice' automation does make perfect sense in Peril/Danger, it can be a bit confusing to develop on and test. Additionally there seems to be some kinks left to work out (the deleting comment thing).

Since Horizon, which started to visualize release pipelines, is now expanding to do some automation around releases, maybe this enhancement (as it's explicitly about releases), makes sense to roll into there?

cc @joeyAghion @dblandin

From heroku:

2018-01-26T19:00:19.968593+00:00 heroku[router]: at=info method=POST path="/webhook" host=artsy-peril.herokuapp.com request_id=aaad6169-2a00-4d23-98ea-231c1ced7b6b fwd="192.30.252.38" dyno=web.1 connect=0ms service=2751ms status=200 bytes=352 protocol=https
2018-01-26T19:00:19.963456+00:00 app[web.1]: info: [runner] - Commenting, with results:
2018-01-26T19:00:19.963467+00:00 app[web.1]: mds: 0
2018-01-26T19:00:19.963470+00:00 app[web.1]: messages: 0
2018-01-26T19:00:19.963471+00:00 app[web.1]: warns: 0
2018-01-26T19:00:19.963475+00:00 app[web.1]: fails: 0
2018-01-26T19:00:19.963477+00:00 app[web.1]:
2018-01-26T19:00:20.033823+00:00 app[web.1]: Error:  Error {
2018-01-26T19:00:20.033828+00:00 app[web.1]:   message: '{"message":"Not Found","documentation_url":"https://developer.github.com/v3/repos/contents/#get-contents"}',
2018-01-26T19:00:20.033829+00:00 app[web.1]:   code: 404,
2018-01-26T19:00:20.033832+00:00 app[web.1]:   status: 'Not Found',
2018-01-26T19:00:20.033834+00:00 app[web.1]:   headers:
2018-01-26T19:00:20.033836+00:00 app[web.1]:    { server: 'GitHub.com',
2018-01-26T19:00:20.033838+00:00 app[web.1]:      date: 'Fri, 26 Jan 2018 19:00:20 GMT',
2018-01-26T19:00:20.033841+00:00 app[web.1]:      'content-type': 'application/json; charset=utf-8',
2018-01-26T19:00:20.033843+00:00 app[web.1]:      'content-length': '106',
2018-01-26T19:00:20.033844+00:00 app[web.1]:      connection: 'close',
2018-01-26T19:00:20.033846+00:00 app[web.1]:      status: '404 Not Found',
2018-01-26T19:00:20.033848+00:00 app[web.1]:      'x-ratelimit-limit': '12500',
2018-01-26T19:00:20.033850+00:00 app[web.1]:      'x-ratelimit-remaining': '12382',
2018-01-26T19:00:20.033852+00:00 app[web.1]:      'x-ratelimit-reset': '1516995298',
2018-01-26T19:00:20.033853+00:00 app[web.1]:      'x-github-media-type': 'github.machine-man-preview; format=json',
2018-01-26T19:00:20.033858+00:00 app[web.1]:      'access-control-expose-headers': 'ETag, Link, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval',
2018-01-26T19:00:20.033859+00:00 app[web.1]:      'access-control-allow-origin': '*',
2018-01-26T19:00:20.033861+00:00 app[web.1]:      'content-security-policy': 'default-src \'none\'',
2018-01-26T19:00:20.033863+00:00 app[web.1]:      'strict-transport-security': 'max-age=31536000; includeSubdomains; preload',
2018-01-26T19:00:20.033865+00:00 app[web.1]:      'x-content-type-options': 'nosniff',
2018-01-26T19:00:20.033867+00:00 app[web.1]:      'x-frame-options': 'deny',
2018-01-26T19:00:20.033869+00:00 app[web.1]:      'x-xss-protection': '1; mode=block',
2018-01-26T19:00:20.033871+00:00 app[web.1]:      'x-runtime-rack': '0.049933',
2018-01-26T19:00:20.033872+00:00 app[web.1]:      'x-github-request-id': '9500:2D371:1787FC6:312C238:5A6B7AC3' } }
2018-01-26T19:00:20.034094+00:00 app[web.1]:
2018-01-26T19:00:20.034097+00:00 app[web.1]: /app/out/peril.js:15
2018-01-26T19:00:20.034099+00:00 app[web.1]:         throw reason;
2018-01-26T19:00:20.034101+00:00 app[web.1]:         ^
2018-01-26T19:00:20.034114+00:00 app[web.1]: Error: {"message":"Not Found","documentation_url":"https://developer.github.com/v3/repos/contents/#get-contents"}
2018-01-26T19:00:20.040692+00:00 app[web.1]: info: [CLUSTER] Worker 300 died with code: 1, and signal: null
2018-01-26T19:00:20.040772+00:00 app[web.1]: info: [CLUSTER] Starting a new worker
2018-01-26T19:00:20.565151+00:00 app[web.1]: info: [CLUSTER] Worker 310 is online

Proposal:

Create a GitHub team in our org of people who are interested in getting notifications. Every hour, Peril loops through these people, sees if they have assigned open PRs which that they haven't interacted on.

Peril can then poke them on slack about the PR.

Reasoning

We used to have something like this, now we don't. Now it feels like my PRs get hung by others and I have to remind people that they exist. It was much easier before to send PRs to people who are not actively watching a particular project.

Exceptions:

If you're not in the group, you don't get the message. It'd be opt-in. We can migrate anyone who signed up to the old slackbot by default.

Additional Context:

• We can grab slack IDs for someone via team.artsy.net.

Proposal:

On every PR, check the license of a OSS project to make sure the date range includes the current year.

Reasoning

Every year we go through and update our licenses, @jonallured did a bunch this year:

https://github.com/artsy/eigen/blob/master/LICENSE
https://github.com/artsy/bearden/blob/master/LICENSE
https://github.com/artsy/force/blob/master/LICENSE

We should automate letting people know, this is especially useful at the beginning of the year :P

Exceptions:

I think checking that the LICENSE exists on an open repo is also probably a good idea, I know we probably have an exception or two to that, but those can be handle individually.

Proposal:

On every PR, which has a README at the root, ensure that there's a Meta section

Meta

• State: production
• Point People: @alloy, @orta
• CI :

Fail the PR if meta section is not present or is malformed. Mandatory fields count include CI, point people, state, deploy links.

Reasoning

We have hundreds of projects, we need some consistency.

Proposal:

Your work in GitHub should automate Jira tickets, it should be simple and reflect how we are already working.

The simplest proposal I have is to allow Peril to take;

1. Commit messages with shorthand IDs e.g. "PLAT-89 Turns on the flabberwocky"
2. Shorthand IDs or URLs to jira links in a body

Then use those references to transition the referenced tickets to the "In Progress" state, and to leave a comment passing the link to the PR while the PR is open.

Then when the PR is merged, update the status to "In QA".

Reasoning

Jira is an additional place to be duplicating work, as much of that as possible should be automated.

Exceptions:

I don't think we have complicated enough workflows today, but it's possible in the future we'll need to handle this a bit more like how GitHub does "fixes #123" instead. That can be handled when we get there.

Proposal:

Allow commenting in a PR that it's good to merge assuming CI passes.

Peril can listen to the status updates on a PR, then when they are all green - can check the PR comments to see if anyone say "Merge on green", if they have said it, then merge the PR.

Reasoning

My favourite GitLab feature is a tick box you have during review that this PR should be merged when it passes. I think the flow makes a lot of sense, CI can take a lot of time and remember to go back and merge after is annoying.

An alternative to this is that instead peril can send a slack DM to when it's green.

Exceptions:

It's hard to see this one being triggered by accident, but maybe Peril could confirm that it will merge on green to ensure that people aren't confused?

Proposal:

Apply a spell checker to every markdown document that appears in a PR.

Reasoning

We want to have polished documents, both internally and externally. Having a spellcheck
happening without any effort on a developers part means that we'll get a second look at
any documentation improvements on any repo.

Exceptions:

This won't be perfect, but it is better to get something working than to not have it at all.
I added the ability to ignore files: so CHANGELOGs which tend to be really jargon heavy will
be avoided in every repo.

Other than that, we can continue to build up a global list of words to ignore.

Additional Context:

You can see our discussion in slack here

Every repo should have a way to contribute to it with a CONTRIBUTING doc. We tend to put these in README often, then move them as they tend to get long.

Proposal:

Migrate from graphql-slack-updater to a weekly peril job.

Reasoning

• Consolidation of tooling
• Less code
• Easier for others to contribute
• Moves from running on CI to in-memory ( artsy/graphql-slack-updater#1 )

Exceptions:

None, as each GraphQL server would need to be added individually. So start with metaphysics, and wait depending on how far stitching is along.

Additional Context:

Will be the first time that Peril uploads something to S3, so will need to add support for that..

For devs: GitHub event ID: ac11df00-0366-11e8-991a-ee17164aa737

Proposal:

Move Danger / Peril from an "opt-out" model to an "opt-in" model. Repos would not be explicitly ignored via the settings.ignored_repos config directive, but Danger / Peril would only process repos that have a Dangerfile present.

Additionally or alternatively, repos that have a Dangerfile present would explicitly import rulesets from our global organization. I.e. a Dangerfile in a given project repo would need to include the config directive rules.pull_request.* for matching rules at the organization level to be applied.

I am not sure this is the right place to raise this as it relates to the way Danger applies rules in itself, or what specific implementation can come out of this as a rule, but wanted to open the RFC as advised.

Reasoning

My reasoning is that I want to have control over which organizational-level and project specific rules apply to a given project. When I create a new project, I want to include a Dangerfile specifying exactly what behavior I believe is appropriate for that project. I think an opt-in model would allow us to tailor Danger / Peril's behavior to suit our project's specific needs.

Exceptions:

I can see that we may want to keep certain workflow rules applied at the organizational level, regardless, such as "assign someone to this PR".

Additional Context:

The issues I raised around Danger / Peril being applied across all repos in our organization start in Slack here

Proposal:

Once a day, ideally in the morning, Peril runs a job that shows how many commits are in staging but are not deployed to master.

Reasoning

Metaphysics staging today is 9 days different from production, it would have been nice to keep track of that difference over time rather than when someone noticed that a bug wasn't fixed.

As we should be using the same naming conventions everywhere it's a matter of looking at the diff between master and production (or something like that)

This would be set up as a map, so artsy/metaphysics => #graphql, artsy/gravity => #frontend-notifications.

Exceptions:

Opt-in, so no exceptions.

Proposal:

When a PR is closed, check the labels applied to the PR - then do a look up for the label to specific channels.

Reasoning

For example: Consignments or Auctions, they both have labels in Eigen/Emission/Reaction but they don't have corresponding notifications in their notification/dev channels for those repos because it's too much noise. This can be fixed it by Peril sending a slack message for the merged PR in their channels.

Exceptions:

Off-hand, none that I can think of, it's mostly just a mapping of a specific label to a slack channel. If the mapping doesn't exist. nothing is sent.

To ensure that you make a PR like this: artsy/README#138

Proposal:

We have a few disparate release notifications, some of them are overly comprehensive

Some feel completely unrelated to the release

Others are just someone saying "I'm making a release of x".

Why not try have something half-way, most projects are already using GitHub tags, so make a channel of all GitHub tag notifications

Reasoning

You can see changes overall as things are deployed

Exceptions:

Nothing I can think of today. Willing to bet a lot of our web/platform-y infra doesn't create tags on deploys, but it's probably a good call for them to do so, and this can push towards that.

See artsy/metaphysics#1291

Proposal:

Add support for #rebaseongreen comment trigger to merge via rebase strategy when status checks are green.

Reasoning

We have #mergeongreen and #squashongreen. Let's support #rebaseongreen as well.

Additional Context:

• Squash on Green PR: #161

This might live inside peril/danger, but:

2019-01-11T17:36:27.062Z  info:
2019-01-11T17:36:27.084Z  info: ## issue_comment.created on artsy on artsy/README
2019-01-11T17:36:27.107Z  info:    1 run needed: artsy/peril-settings@org/markAsMergeOnGreen.ts
2019-01-11T17:36:29.350Z  Fri, 11 Jan 2019 17:36:29 GMT runFromSameHost info: ☢️ Peril at undefined
2019-01-11T17:36:29.410Z
2019-01-11T17:36:29.465Z  Fri, 11 Jan 2019 17:36:29 GMT runFromSameHost info: Started run for artsy/peril-settings@org/mergeOnGreen.ts
2019-01-11T17:36:29.512Z
2019-01-11T17:36:29.572Z  Fri, 11 Jan 2019 17:36:29 GMT runFromSameHost info: ☢️ Peril at undefined
2019-01-11T17:36:29.600Z
2019-01-11T17:36:29.622Z  Fri, 11 Jan 2019 17:36:29 GMT runFromSameHost info: Started run for artsy/peril-settings@org/markAsMergeOnGreen.ts
2019-01-11T17:36:29.644Z
2019-01-11T17:36:30.754Z  Fri, 11 Jan 2019 17:36:30 GMT runFromSameHost Adding the label: { owner: 'artsy', repo: 'artsy', id: 145 }
2019-01-11T17:36:30.797Z
2019-01-11T17:36:31.098Z  Fri, 11 Jan 2019 17:36:31 GMT runFromSameHost Unable to evaluate the Dangerfile
2019-01-11T17:36:31.159Z   { HttpError: {"message":"Not Found","documentation_url":"https://developer.github.com/v3/issues/labels/#create-a-label"}
2019-01-11T17:36:31.197Z      at response.text.then.message (/home/nowuser/src/node_modules/@octokit/rest/lib/request/request.js:72:19)
2019-01-11T17:36:31.254Z      at process._tickCallback (internal/process/next_tick.js:68:7)
2019-01-11T17:36:31.283Z    name: 'HttpError',
2019-01-11T17:36:31.304Z    code: 404,
2019-01-11T17:36:31.325Z    status: 'Not Found',
2019-01-11T17:36:31.346Z    headers:
2019-01-11T17:36:31.367Z     { 'access-control-allow-origin': '*',
2019-01-11T17:36:31.389Z       'access-control-expose-headers':
2019-01-11T17:36:31.410Z        'ETag, Link, Location, Retry-After, X-GitHub-OTP, X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, X-OAuth-Scopes, X-Accepted-OAuth-Scopes, X-Poll-Interval, X-GitHub-Media-Type',
2019-01-11T17:36:31.431Z       connection: 'close',
2019-01-11T17:36:31.452Z       'content-encoding': 'gzip',
2019-01-11T17:36:31.473Z       'content-security-policy': 'default-src \'none\'',
2019-01-11T17:36:31.494Z       'content-type': 'application/json; charset=utf-8',
2019-01-11T17:36:31.516Z       date: 'Fri, 11 Jan 2019 17:36:31 GMT',
2019-01-11T17:36:31.537Z       'referrer-policy': 'origin-when-cross-origin, strict-origin-when-cross-origin',
2019-01-11T17:36:31.558Z       server: 'GitHub.com',
2019-01-11T17:36:31.579Z       status: '404 Not Found',
2019-01-11T17:36:31.600Z       'strict-transport-security': 'max-age=31536000; includeSubdomains; preload',
2019-01-11T17:36:31.622Z       'transfer-encoding': 'chunked',
2019-01-11T17:36:31.644Z       'x-content-type-options': 'nosniff',
2019-01-11T17:36:31.665Z       'x-frame-options': 'deny',
2019-01-11T17:36:31.686Z       'x-github-media-type':
2019-01-11T17:36:31.707Z        'github.v3; format=json, github.machine-man-preview; format=json',
2019-01-11T17:36:31.728Z       'x-github-request-id': 'AB14:8147:8A8BF:A76BD:5C38D41E',
2019-01-11T17:36:31.750Z       'x-ratelimit-limit': '12500',
2019-01-11T17:36:31.771Z       'x-ratelimit-remaining': '12256',
2019-01-11T17:36:31.792Z       'x-ratelimit-reset': '1547230694',
2019-01-11T17:36:31.813Z       'x-xss-protection': '1; mode=block' } }

Add checks for 🍏 📗 💚 🥗 (raw emojis) and 🍏 📗 💚 🥗 (github emojis) in mark as merge on green

There's a race condition where peril checks for assignees before renovate has a chance to add them. It'd be nice to just disable that check for renovate PRs.

Proposal:

Whenever a PR is created in Eigen and is ready, check the PR description changelog changes. The outcome of this should be:

• Validate the changelog changes and notify the author if something is wrong
• Clear feedback about all the changes

Examples:

Reasoning

The rule should enforce all of the invariants described in this RFC and log all the changelog entries that will be added (for debugging purposes in case something goes wrong).

Exceptions:

• This rule will be running only on Eigen
• This rule should be only turned on when the related work is enabled in Eigen. We can achieve that by checking for a string in the PR body #run_new_changelog_check.

Resolution:

• Merge this #183

Proposal:

• Expand support for the RFC webhook to move from just this repo, to all Artsy repos
• Support RFC: and [RFC] as we use both in practice

Reasoning

Currently, if you define an RFC on any other repo

• artsy/emission#934
• artsy/eigen#2444
• https://github.com/artsy/potential/issues/127

Then it stays in just that repo. Let's raise the visibility for all of these to #dev.

Exceptions:

Good Q, offhand I don't see any reasons to have a repo with an exception to this. If one repo has a lot of of RFCs which aren't generally available then that can be built

Additional Context:

There isn't a direct slack discussion, but it's part of #39 I think.

Proposal:

Warn that a PR that is not classed as a WIP needs an assignee.

Reasoning

This rule is in most Dangerfiles in our org, and is generally a pattern everyone uses due to our
slack bot integration. This formalises it to all repos.

Exceptions:

This is probably worth having as a warning for a long time, then moving to a failure once we're all used to the pattern - like 6 months kind of time.

In Slack today, starting here, there was a discussion around the usefulness of Peril and Danger that led to the realization that the RFC process is currently inadequate. Developers expressed that the RFC process was opaque and too top-down. There were also concerns raised that the RFC process currently excludes (and fails to include) remote developers.

#38 clarifies the existing RFC process, but we should have a deeper discussion to make sure that everyone at Artsy feels that they're included in the process.

As a refresher, the RFC process currently is:

• Someone opens an issue on this repo that begins with "RFC: ", and that causes a message to be posted to Slack's #dev room.
• The RFC proposer announces the RFC in a weekly dev standup.
• At least a week goes by for discussion.
• Based on the outcome of the discussion, the RFC is implemented in a pull request, which is also open for discussion.

Here are a few questions to kick off the discussion:

• Is there anything missing from the RFC process? I suggested adding a reminder to Slack, for example.
• How can we further empower individual engineers to participate more in discussions and to propose their own RFCs?
• How can we make sure that the RFC process actively includes all team members, with a focus on remote folks, but also includes everyone in terms of seniority or team membership?

If you'd like to express an opinion privately (this repo is open source and public) please don't hesitate to reach out privately to me on Slack.

When creating this PR: https://github.com/artsy/gravity/pull/11733 Peril:

• Ran the same spellcheck 5 times and left 5 identical comments
• Spellchecked deleted sections of code

Proposal:

Disable the Peril rule requiring that there be an assignee for deployment PRs (PRs from master -> release)

Reasoning

A deployment PR isn't an artifact that should require review - in repositories where we have PR-based deployments, we are just leveraging the CI hooks related to PRs to drive an automated deploy. So in this case, the Peril warning isn't needed, so it should be removed.

Exceptions:

I don't think there should be any?

Additional Context:

Here's an example PR demonstrating the problem: https://github.com/artsy/candela/pull/187

Proposal:

It's possible to make a PR without any additional context. We should request that anyone making a PR with an empty body should add some text.

Reasoning

PR metadata is an important part of understanding how/why something happened later on, skipping a writeup or a quick link to a slack conversation makes it much harder to grok Artsy as a collection of systems.

Exceptions:

None that I can think of today? I was wondering about a revert PR, but even those could do with a "why".

Proposal:

Let's say you had a commit like this:

Which is the commit the format used inside GMV. Peril should look through all of the existing labels for that repo and apply any that are used inside the commit message as a [Label] match.

Reasoning

This data already exists in issues and commits, so adding it here means less chore work.

Exceptions:

This won't fail builds, and is only additive - so no need for exceptions

Platform leads have an update section in open engineering standup.

Send them reminders an hour before the meeting to collect their notes.

I wouldn't say I'm strongly in favor of this, but I'm not sure how much value it adds or whether it's worth the time to chase completeness in adding new terminology, yet it's rather distracting.

Currently, there is an N+1 issue for fetching PR's from a list of commits:

• fetch list of commits/
• for each commit, fetch relevant PR id
• (if PR not fetched yet, fetch PR info for id)

We can use a GraphQL API to consolidate this all to one request.

One easy way to access the GraphQL API, from the octokit REST API wrapper made available via danger, is described in https://github.com/octokit/rest.js/issues/592#issuecomment-373387697 . Alternatively, one could add support for exposing the GraphQL API wrapper from octokit, in danger.

cc @dblandin

Proposal:

On every PR, which has a meta section like this:

Meta

• State: production
• Point People: @alloy, @orta
• CI :

This is a core Artsy Mobile OSS project, along with Energy, Eidolon, Emission and Emergence.

Fail the PR if the people who are assigned are still inside the Artsy org.

Reasoning

Code ownership changes naturally, but forgotten repos can get lost and no-one want to have orphaned. As people leave we should be re-applying who has ownership over the repo.

Failing is a strong call, but leaving the repo in a bad state isn't a great call either.

Exceptions:

There are probably a bunch of repos without the meta section. The meta section should ideally get added too them, but I think there's too many edge cases to make it worth forcing it on every repo. Open to changing that though.

in /spellcheck.json
per artsy/README#427

Proposal:

Close stale PRs after a period of inactivity.

When a PR is inactive for a certain period, we add a label to mark it as 'stale' and a comment to let the author know about it. If someone (the author or someone else) comments in the PR ("still waiting for review" or "work on progress"...), then we remove the stale label.

This is close to the same behavior one gets from Stale but it will mean we will have to implement it in each repo on its own since GitHub organization wide actions is still limited now

Reasoning

Over 50% of open PRs in Eigen, Force, Metaphysics and Volt are stale for a few weeks/months. Most of those PRs are probably abandoned. We want to keep only PRs that are actionable and that might make it to the main branch open and close the stale PRs. PS: Closed PRs won't go anywhere, we still can open them again if needed.

Exceptions:

• I am not sure if there should be any exceptions here but we can initially start by applying this only to the repos mentioned above (Eigen, Force, Metaphysics and Volt).
• Sometimes, the author of the PR stops working on a PR and gets back to it after a few weeks for example if something else more urgent needs to work on. I suggest we add a custom tag #disablestalecheck to avoid having Danger close the PR

Resolution:

• Create a poll in the dev channel to ask for the duration of inactivity we want to set
• Add the new rule to Danger

Feel free to edit this comment to keep a running list of some enhancements to the Github comments. Currently looks like:

Some thoughts:

• consolidate successive bumps of the same library into one line (listing the full span of version changes)
• for Reaction lib updates, include relevant Reaction PR's (should try to use renovate API)?

cc @dblandin

I would like to be able to opt-out of Danger checks on certain file paths, much like a .gitignore file we should be able to put a .dangerignore file in our repos to exclude glob'ed paths.

Send periodic updates that say "It's been X days since a blog post has been published."

cc @ashfurrow what do you think?

Proposal:

Require changes to code to be accompanied by a change to a changelog entry (if applicable).

Reasoning

Many projects under the Artsy org have changelogs, and we are currently replicating Danger configs to require changelog entries (example). We should consider standardizing this across the org using Peril.

Exceptions:

If there's no changelog in the repo, the rule should not apply.

Additionally, the rule could be smart about what constitutes a "code change." For example, a PR that only adds unit tests shouldn't require a changelog entry. I don't have precise logic for what this would look like, but it would be doable with a few "if" statements I'm sure. We can also update the rule to handle edge cases as they occur.