danger / danger

🚫 Stop saying "you forgot to …" in code review (in Ruby)

Danger 🚫

Formalize your Pull Request etiquette.

---

What is Danger? • Vision • Helping Out • Plugin Development

---

What is Danger?

Danger runs during your CI process, and gives teams the chance to automate common code review chores.

This provides another logical step in your process, through this Danger can help lint your rote tasks in daily code review.

You can use Danger to codify your teams norms. Leaving humans to think about harder problems.

For example?

You can:

• Enforce CHANGELOGs
• Enforce links to Trello/JIRA in PR/MR bodies
• Enforce using descriptive labels
• Look out for common anti-patterns
• Highlight interesting build artifacts
• Give specific files extra focus

Danger provides the glue to let you build out the rules specific to your team's culture, offering useful metadata and a comprehensive plugin system to share common issues.

Getting Started

Alright. So, actually, you may be in the wrong place. From here on in, this README is going to be for people who are interested in working on and improving on Danger.

We keep all of the end-user documentation at https://danger.systems.

Some quick links: Guides Index, DSL Reference, Getting Started and What does Danger Do?.

I'm here to help out!

Brilliant. So, let's get you set up.

git clone https://github.com/danger/danger.git
cd danger
bundle install
bundle exec rake spec

This sets everything up and runs all of the tests.

Theory

Danger has a VISION.md file, which sums up the ideas around what Danger is. It is the lower bounds of what Danger means. Orta has written on handling and creating Danger on the Artsy blog, too.

Documentation

The code you write may end up in the public part of the website — the easiest way to tell is that it is vastly overdocumented. If you are working in a space that looks over-documented, please be extra considerate to add documentation. We expect the consumers of that documentation to be non-rubyists, thus you should avoid specific jargon and try to provide duplicate overlapping examples.

Testing

So far, we've not really figured out the right way to make tests for our CLI commands. When we have done so, they've ended up being brittle. So, ideally, try to move any logic that would go into a command into separate classes, and test those. We're okay with the command not having coverage, but ideally the classes that make up what it does will.

I'd strongly recommend using bundle exec guard to run your tests as you work. Any changes you make in the lib, or specs will have corresponding tests run instantly.

Debugging

Ruby is super dynamic. One of the best ways to debug Ruby code is by using pry. We include pry for developers: when you have a problem, copy these two lines just before your problem and follow the instructions from "I Want To Be A Danger Wizard."

require 'pry'
binding.pry

License, Contributor's Guidelines and Code of Conduct

We try to keep as much discussion as possible in GitHub issues, but also have a pretty inactive Slack --- if you'd like an invite, ping @Orta a DM on Twitter with your email. It's mostly interesting if you want to stay on top of Danger without all the emails from GitHub.

This project is open source under the MIT license, which means you have full access to the source code and can modify it to fit your own needs.

This project subscribes to the Moya Contributors Guidelines which TLDR: means we give out push access easily and often.

Contributors subscribe to the Contributor Code of Conduct based on the Contributor Covenant version 1.3.0.

danger's Issues

Usage not clear

Hi.

I'm finding it hard to understand what this actually does.
The readme says In CI run bundle exec danger. This will look at your Dangerfile and provide some feedback based on that.

Can you define some feedback in a more clearer way :)

Move `danger init` example content into assets folder

Make `danger init` a wizard

README should recommend:

• Making Gemfile

    gem 'danger'

• Doing a bundle install
• Starting the Wizard via bundle exec danger init

Then the steps will guide you through the process of

1. make new account for the repo
2. Talking you through the access settings and what it means
3. setting up a new access token for repo ( Note, differentiate between OSS / Closed Source )
4. creating a default Dangerfile
5. Offering some ideas for further expansion
6. Talk about how to add Danger to your CI

Running danger without write access to the repo

Currently when running danger on an open source project with a key set up to not have write access I run into

Found 2 error(s) and I don't have write access to the PR set a PR status. bundle exec danger returned exit code 1

As far as I remember we had a way to not set the return status for this situation. What's the best way to solve this? I'll submit a PR to both the setup and the docs to make this easier, just wanted to check what's your thoughts @orta 👍

Detect important Info.plist changes

E.g., don’t allow a bundle display name of ‘Artsy β’ to be checked-in.

README should explain how to get your first PR running with danger

Use 100% width tables for messages

As of the latest build messages look like:

      | 3 Errors
    ------------- | -----------------------------------------------------------------------------------------------------
        :no_entry_sign: | fdescribe left in tests
        :no_entry_sign: | fit left in tests
        :no_entry_sign: | Developer Specific file shouldn't be changed

	3 Errors
🚫	fdescribe left in tests
🚫	fit left in tests
🚫	Developer Specific file shouldn't be changed

This leads to ugly jagged edges on when there's errors/fails/messages. However, with some sneaky HTML skills, we can do this:

	3 Errors
🚫	fdescribe left in tests
🚫	fit left in tests
🚫	Developer Specific file shouldn't be changed

<table>
  <thead>
    <tr>
      <th width="50">&nbsp;</th>
      <th width="900">3 Errors</th>
     </tr>
  </thead>
  <tbody>
    <tr>
      <td>:no_entry_sign:</td>
      <td>fdescribe left in tests</td>
    </tr>
    <tr>
      <td>:no_entry_sign:</td>
      <td>fit left in tests</td>
    </tr>
    <tr>
      <td>:no_entry_sign:</td>
      <td>Developer Specific file shouldn't be changed</td>
    </tr>
  </tbody>
</table>

The numbers, once larger than the width of the parent become relative sizes, so if github change the size of their pages, then this will handle it.

Plugin System

Because we can 👯

Add a function to the DSL that outputs all the known DSL attributes + their values

Before I've done something like this in the Dangerfile:

message "Insertions: #{insertions}"
message "Added: #{files_added}"
message "LOC: #{lines_of_code}"
message "Mod: #{files_modified}"

Which isn't great. Instead I should be able to add something like message_all_attributes or something. It would show all of the information in a nice table.

Support locked files

unless has_org_access? "Artsy"
  protect_files(path: "**/*.gemspec", message: "Submit an issue instead of changing dependencies via PRs")
end

Define files that can only be modified by developers with write access to the repo.

Create a Dangerbot webservice

Could be something I can pull off as a MVP with a few weekends I've been wanting to try rails 5

• A bit like the Facebook mention bot
• Pay £5 per user account (e.g. pay once for ORStackView and ARAnalytics) or an org (Eigen, Eidolon, Energy etc ) to allow it to do PR messages / status updates.
• Runs danger in a sandbox only allowing access to the folder it's currently in.
• Would make it easier to set up for orgs, just tick a box and add a Dangerfile.
• Could make a Dangerfile for a collection of repos.

[init] Doesn’t work

It’s expecting a Dangerfile to exist.

Delete existing message if no `message` or errors/warnings on a run

Silence is golden?

lines_of_code is wrong?

I have a PR with 5 additions and 6 deletions. - the lines of code is classed as bigger than 50 in my Dangerfile

Linked files

dependency(changed: "**/*.gemspec", requires: "./lib/danger/version.rb")

Don't show the warning about not being able to set the build status for OSS repos

Relates to #105

I think we should have this information in the GitHub Repo / PR status? or maybe in an environment var?

Printing warning 'Tests were not updated'
Raising error 'Please include a CHANGELOG entry'
Danger has failed this build. 
Found 1 error(s) and I don't have write access to the PR set a PR status.

Want to clean-up this: I don't have write access to the PR set a PR status. so that it doesn't show when it is't needed.

PR Body could move in to the DSL attributes table?

Experiment: if instead of putting pr.body - we chucked in pr.body.lines we might get it working in the terminal. We could also run it through the markdown parser.

Might work?

Customize the merge branch

Today, danger only runs on PRs against master (at least the README says so).

It'd be awesome to be able to customize the merge branch. We, for example, keep a shipped version on master and all the development goes on a development branch.

I can try to take a look at this, but it seems it should be easy. Unless I'm missing something, only two places uses master hardcoded (https://github.com/danger/danger/blob/master/lib/danger/ci_source/buildkite.rb#L20 and

danger/lib/danger/scm_source/git_repo.rb

Line 9 in df58dcb

def diff_for_folder(folder, from = "master", to = 'HEAD')

).

I think one important question is where this branch would be specified: on the Dangerfile (if so, this probably would be harder because it seems that the file actually is run) or as a command line parameter.

Remove <p> tags from table

The <p> tags make the text not centered on each cell, as seen in artsy/eigen#1185 (comment)

This was a side effect of #65

Ensure that all comments are downloaded

I wonder if in this PR Danger has paged off at 5 comments, and now continues to re-post

README needs to explain how to set up auth

Filter out merge commits from the files_modified / added / removed etc

vs

Remove nap dependency

Replace it with Faraday, given that github uses that.

New HTML tables are indented slightly, so GitHub treats them as code

I will comment the HTML for one after this

Use a real table for showing the verbose information

Mine is really not a good one. Recommendations have been the terminal-table gem. I'm interested in also getting Cork up and running, so we can test it outside of CP.

Danger command line output for failing builds needs improvement

Printing warning 'Tests were not updated'
Raising error 'Please include a CHANGELOG entry'
Danger has failed this build. 
Found 1 error(s) and I don't have write access to the PR set a PR status.

Perhaps move to a markdown checklist:

Warnings:
 - [ ] Tests were not updated

Errors:
  - [ ] Please include a CHANGELOG entry 

Danger will finish with a failure build code because of 1 error. 

Circle CI's commit range is a subset of the PR

This was the diff for this PR git diff f12ea82b719a 9d9fece70aac - see build

We need to find the first commit which in that PR could be either 608f2d71a0ac3c88d7681c464ec0cbe9fa5e319d or it's parent c1af413

the env vars from Circle are:

  CIRCLE_ARTIFACTS=/tmp/circle-artifacts.oihhrPZ
  CIRCLE_BRANCH=orta-deploy
  CIRCLE_BUILD_NUM=1476
  CIRCLE_COMPARE_URL=https://github.com/artsy/eigen/compare/f12ea82b719a...9d9fece70aac
  CIRCLE_NODE_INDEX=0
  CIRCLE_NODE_TOTAL=1
  CIRCLE_PREVIOUS_BUILD_NUM=1471
  CIRCLE_PROJECT_REPONAME=eigen
  CIRCLE_PROJECT_USERNAME=artsy
  CIRCLE_SHA1=9d9fece70aacd8e01f9b316664e4180629910f5f
  CIRCLE_TEST_REPORTS=/tmp/circle-junit.R5MI8iJ
  CIRCLE_USERNAME=orta

Comment based fail suppression?

Looking a real example: artsy/eigen#1236

In this case I definitely know that this will fail the build, but I want this to be correct. What about if inside the message for the PR I wrote:

Danger: Ignore "Developer Specific file shouldn't be changed"

Wherein it won't fail the build if a fail is found with the same string.

Ideas / Opinions?

Support for local development

It should be possible to run ./bin/danger locally to setup the initial Dangerfile

Danger's diff strategy is confusing compared to the GH visual web diff

Say I had three commits.

• I wrote some app code
• I git pull origin master
• I wrote some more app code

but I never added a changelog entry.

If the pull contained a changelog entry, it would show up as a file in Danger, but not in the GH UI.

Allow posting comments regardless of ability to change PR status

Currently it won't write a message. You can see it in #47

Status update should say how many failures

What about:

• Fails: 1 Error. 1 Warning. Everything is fixable. (which is Danger's motto) (don't show warning/errors if no warnings or errors )
• Success: Everything is okay.

I'm going to ask her about the wording now ;)

The default template should inspect the current folder to offer smarter defaults

We should have templates that adapt to the repo that init was run in:

• A Gem
  • Warns when Gemspec is changed? We talked about this for Fastlane/Danger
  • There isn't a linter, but we could try eval the gemspec?
• An npm module
• Ruby
  • Look for puts, pry inside app
  • Note if the LOC in an old Gemfile.lock is greater than the new Gemfile.lock
• Rails apps (inherit ruby)
  • Changes to config files without mentioning "config" in the description
• Sinatra projects (inherit ruby)
• Node apps
• React specific apps
• A Pod / SPM module
  • Lint the pod?
  • Note if the LOC in an old Podfile.lock is greater than the new Podfile.lock
• iOS App
• Go projects?

Note: This is a living document and I'll keep updating with ideas here.

Change footer text

Instead of

Generated by danger on 2016-01-08

What about just: [:no_entry_sign:anger](https://github.com/KrauseFx/danger/) (right aligned)

The date is already on the issue, and it's cool that a bot generated the copy, but this can be snappy

🚫anger

Failures should cause an exit code of not 0

POSIX-compatible systems typically use a convention of zero for success and non zero for error

Looks like convention is here - perhaps
78? - EX_CONFIG (78) Something was found in an unconfigured or miscon-figured state.

Circle CI can sometimes create a commit build for pull requests

This CI build https://circleci.com/gh/artsy/eigen/1501 - is attached to this PR: artsy/eigen#1130

it shows that at the top, but the ENV vars don't reflect that that:

Setting build environment variables
  CIRCLE_ARTIFACTS=/tmp/circle-artifacts.gyNRoD5
  CIRCLE_BRANCH=orta-dm_pilot
  CIRCLE_BUILD_NUM=1501
  CIRCLE_COMPARE_URL=https://github.com/artsy/eigen/commit/1b064fe71ac7
  CIRCLE_NODE_INDEX=0
  CIRCLE_NODE_TOTAL=1
  CIRCLE_PREVIOUS_BUILD_NUM=1500
  CIRCLE_PROJECT_REPONAME=eigen
  CIRCLE_PROJECT_USERNAME=artsy
  CIRCLE_SHA1=1b064fe71ac7ec073223221889d96dcecb292000
  CIRCLE_TEST_REPORTS=/tmp/circle-junit.X2xv4ph
  CIRCLE_USERNAME=orta

our validation:

def self.validates?(env)
  return !env["CIRCLE_BUILD_NUM"].nil? && !env["CI_PULL_REQUEST"].nil?
end

Conform to Moya Contributors etc etc

Similar to how RxSwiftCommunity works

Add support for enterprise github

Should be a few minor points to change.

• add support for setting a custom GH host
• make sure that host is passed into the Octokit config

Add List of CIs supported to README

Hey @orta could you please list out the CIs that are supported in the README.md! thx!

Benno

danger local crashing due to missing repo_slug

Haven't had time to look into this yet, I get the following error:

~/Developer/fastlane/danger/bin/danger local --verbose

Danger local requires a repository hosted on github.
Not a Pull Request - skipping `danger` run
/Users/fkrause/Developer/fastlane/danger/lib/danger/commands/local.rb:25:in `run': undefined method `repo_slug' for nil:NilClass (NoMethodError)
    from /Users/fkrause/.rbenv/versions/2.2.1/lib/ruby/gems/2.2.0/gems/claide-1.0.0.beta.1/lib/claide/command.rb:312:in `run'
    from /Users/fkrause/Developer/fastlane/danger/bin/danger:5:in `<main>'

Look into editing an existing message instead of deleting and creating a new one

You get an email for every commit that runs tests in a PR, which isn't optimal.

Make danger detect if its not on a PR commit

a commit build should not run danger, but it shouldn't raise

Support file globbing

Look at artsy/elan#5

WATCHED_FILES = [
  '/components/lib/variables/colors.json',
  '/components/lib/variables/typography.json',
  '/components/lib/variables/widths.json'
]

message("@orta something changed in elan!") if (modified_files & WATCHED_FILES).any?

It would be much nicer to do either:

message("@orta something changed in elan!") if modified_files["/components/lib/variables/*"]

or

message("@orta something changed in elan!") if modified_files.include? "/components/lib/variables/*"

E.g. we could use a special array subclass that has a different include? here.

Provide an API for comparing a file between the two revisions

For example:

compare_file "Podfile.lock" do |before, after|
  reduced_deps =  ":tada: - Great work on reducing our incidental complexity"
  message(reduced_deps) if before.lines_of_code < after.lines_of_code
end

I wonder what attributes could be available?

Rugged requires CMake + Native Extensions which do not work by default on OS X

This is noted in the README: https://github.com/libgit2/rugged#install

Allow passing a github auth token

For private repos

Switch to that table gem

Cork isn't ready for usage as a full dependency.

Use danger on danger

Incorrect stats on CI build

On this build, I got the following output:

Danger v0.3.0
DSL Attributes:
------------------
| lines_of_code  | 4
| files_modified | ["Artsy/View_Controllers/Auction/AuctionInformationViewController.swift"]
| files_removed  | []
| files_added    | []
| deletions      | 4
| insertions     | 0
| pr_title       | Adds Swift abstraction for Navigation Buttons
| pr_body        | This abstracts the messiness of working with the `ARNavigationButton` stuff with Swift into one place.

Despite the large-ish patch

Expose PR labels

I'd be nice to have a custom variable in DSL to get the PR labels. Probably not super important, since you should be able to get that from env.request_source.pr_json.