Initial Danger integration #1036

marcelofabri · 2016-12-23T00:29:46Z

Fixes #843

@jpsim We need to create a bot account and a token with it, then add it as an env var (DANGER_GITHUB_API_TOKEN)

From danger init:


Step 2: Creating a GitHub account

In order to get the most out of Danger, I'd recommend giving her the ability to post in
the code-review comment section.

IMO, it's best to do this by using the private mode of your browser. Create an account like
SwiftlintBot, and don't forget a cool robot avatar.

Here are great resources for creative commons images of robots:
 -> https://www.flickr.com/search/?text=robot&license=2%2C3%2C4%2C5%2C6%2C9
 -> https://www.google.com/search?q=robot&tbs=sur:fmc&tbm=isch&tbo=u&source=univ&sa=X&ved=0ahUKEwjgy8-f95jLAhWI7hoKHV_UD00QsAQIMQ&biw=1265&bih=1359
SwiftlintBot will need access to your repo. Simply because the code is not available for the public
to read and comment on.

Note: Holding cmd ( ⌘ ) and double clicking a link will open it in your browser.

Cool, please press return when you have your account ready (and you've verified the email...)


Step 3: Configuring a GitHub Personal Access Token

Here's the link, you should open this in the private session where you just created the new GitHub account
 -> https://github.com/settings/tokens/new

For token access rights, I need to know if this is for an Open Source or Closed Source project
? [ Open / Closed ]
> 
Using: open
For Open Source projects, I'd recommend giving the token the smallest scope possible.
This means only providing access to public_repo in the token.

This token limits Danger's abilities to just writing comments on OSS projects. I recommend
this because the token can quite easily be extracted from the environment via pull requests.

It is important that you do not store this token in your repository, as GitHub will automatically revoke it when pushed.

👍, please press return when you have your token set up...


Step 4: Add Danger for your CI

I'd recommend adding before_script: bundle exec danger to the script section of your .travis.yml file.
You shouldn't use after_success, after_failure, after_script as they cannot fail your builds.

OK, I'll give you a moment to do this...


Final step: exposing the GitHub token as an environment build variable.

As you have an Open Source repo, this token should be considered public, otherwise you cannot
run Danger on pull requests from forks, limiting its use.
In order to add an environment variable, go to:
 -> https://travis-ci.org/marcelofabri/SwiftLint/settings

The name is DANGER_GITHUB_API_TOKEN and the value is the GitHub Personal Access Token.
Make sure to have "Display value in build log" enabled.
This is the last step, I can give you a second...

marcelofabri · 2016-12-23T00:34:41Z

I've done this a first step towards #1025.

jpsim · 2016-12-23T00:39:59Z

I've done this a first step towards #1025.

I'm not exactly sure how this helps with #1025. You're thinking we should run OSS-check as a "Danger job" in order to piggy-back off its auto-updating GitHub comment reporting capabilities?

marcelofabri · 2016-12-23T00:43:37Z

Yes! My idea would be having a custom formatter as you hinted and then reading it and commenting with danger. Or we could use the JSON formatter as danger-swiftlint does and format it (with links to the files in their repos for example).

We'd need to make danger run in the end though. Or we could have two Dangerfiles - one for PR validation (in this PR), which could be run in any job; and another one that runs after the OSS-check.

marcelofabri · 2016-12-23T01:01:11Z

We can have something like this:

	Realm Swift
🚫	Colon Rule Violation: Colons should be next to the identifier when specifying a type and next to the key in dictionary literals. RealmSwift/Tests/ObjectCreationTests.swift#L463

Unfortunately, checkboxes don't work inside tables, so we'd need another way to mark a violation as legit.

Actually, even if it did danger would delete it on a new build 😬

jpsim · 2016-12-23T01:06:54Z

How hard would it be to build our own auto-updating GitHub comment reporting, which would actually allow the format we want? It doesn't seem too challenging, but I may be ignorant of the complexity involved...

marcelofabri · 2016-12-23T01:08:33Z

Danger does support any markdown text. Or are you thinking about something else?

jpsim · 2016-12-23T01:09:37Z

Ah, I see. I thought it was necessary to use the table format. But that still doesn't help us avoid overwriting "approved" violations.

marcelofabri · 2016-12-23T01:21:53Z

Yeah, we could get away with that by using GitHub API and applying our own logic in Dangerfile though.

codecov-io · 2016-12-23T01:25:11Z

Current coverage is 83.15% (diff: 100%)

Merging #1036 into master will not change coverage

@@             master      #1036   diff @@
==========================================
  Files           145        145          
  Lines          7098       7098          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           5902       5902          
  Misses         1196       1196          
  Partials          0          0

Powered by Codecov. Last update 82d65c3...7c07b44

jpsim · 2016-12-24T21:33:10Z

I've created @SwiftLintBot, a new token for that account and added it in Travis's DANGER_GITHUB_API_TOKEN env vars. Then I reran the SPM build that had previously failed, which has now succeeded, but I was expecting Danger to post a comment, which it did not. Although maybe that's normal because none of the code paths causing fail or warn in the Dangerfile were hit, so Danger doesn't comment in those cases?

marcelofabri · 2016-12-24T21:39:54Z

Yes, that's the expected behavior. You can run it locally against a PR that would fail to check:

danger pr https://github.com/realm/SwiftLint/pull/1032

jpsim · 2016-12-24T21:57:32Z

Dangerfile

+    markdown <<-MARKDOWN
+Here's an example of your CHANGELOG entry:
+```markdown
+* #{github.pr_title}#{' '}


Why not this?

-* #{github.pr_title}#{' '} -[#{github.pr_author}](https://github.com/#{github.pr_author}) -[#issue_number](https://github.com/realm/SwiftLint/issues/issue_number) +* #{github.pr_title} + [#{github.pr_author}](https://github.com/#{github.pr_author}) + [#issue_number](https://github.com/realm/SwiftLint/issues/issue_number)

because I shamelessly copied this from CocoaPods' Dangerfile 😂

I've kept the #{' '} to make sure we don't miss this if someone uses an editor that strips trailing whitespaces.

that's just one space not two, though?

Just curious, why the two spaces at the end of the github.pr_title line?

To force a newline.

It was taken from CocoaPods.

jpsim · 2016-12-24T21:59:02Z

.travis.yml

@@ -13,6 +13,7 @@ matrix:
      env: JOB=SPM
      os: osx
      osx_image: xcode8.2
+      before_script: bundle install --jobs=3 --retry=3 --without documentation --path bundle && bundle exec danger


maybe break this up into a YAML array for legibility?

before_script: - bundle install --jobs=3 --retry=3 --without documentation --path bundle - bundle exec danger

Fixes #843

jpsim · 2016-12-24T22:23:21Z

We should probably add .bundle/ and bundle/ to .gitignore?

jpsim reviewed Dec 24, 2016

View reviewed changes

marcelofabri added 3 commits December 24, 2016 20:03

Initial Danger integration

00c5ba0

Fixes #843

Installing bundler

08257f6

PR feedback

986dcb8

Adding bundler folders to .gitignore

7c07b44

jpsim approved these changes Dec 24, 2016

View reviewed changes

jpsim merged commit 9dc0868 into realm:master Dec 24, 2016

marcelofabri deleted the initial-danger branch December 25, 2016 00:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial Danger integration #1036

Initial Danger integration #1036

marcelofabri commented Dec 23, 2016

marcelofabri commented Dec 23, 2016

jpsim commented Dec 23, 2016

marcelofabri commented Dec 23, 2016

marcelofabri commented Dec 23, 2016

jpsim commented Dec 23, 2016

marcelofabri commented Dec 23, 2016

jpsim commented Dec 23, 2016

marcelofabri commented Dec 23, 2016

codecov-io commented Dec 23, 2016 •

edited

Loading

jpsim commented Dec 24, 2016

marcelofabri commented Dec 24, 2016

jpsim Dec 24, 2016

marcelofabri Dec 24, 2016

marcelofabri Dec 24, 2016

jpsim Dec 24, 2016

acecilia Jan 21, 2022

jpsim Jan 21, 2022

jpsim Jan 21, 2022

jpsim Dec 24, 2016

marcelofabri Dec 24, 2016

jpsim commented Dec 24, 2016

Initial Danger integration #1036

Initial Danger integration #1036

Conversation

marcelofabri commented Dec 23, 2016

marcelofabri commented Dec 23, 2016

jpsim commented Dec 23, 2016

marcelofabri commented Dec 23, 2016

marcelofabri commented Dec 23, 2016

jpsim commented Dec 23, 2016

marcelofabri commented Dec 23, 2016

jpsim commented Dec 23, 2016

marcelofabri commented Dec 23, 2016

codecov-io commented Dec 23, 2016 • edited Loading

Current coverage is 83.15% (diff: 100%)

jpsim commented Dec 24, 2016

marcelofabri commented Dec 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpsim commented Dec 24, 2016

codecov-io commented Dec 23, 2016 •

edited

Loading