Changelog #1

ashfurrow · 2017-12-31T05:08:40Z

Okay so this PR is the RxSwiftCommunity's first contact with Peril. What's Peril? In short, Peril is a server that runs on Heroku that gets notified by GitHub when new issues or pull requests are opened. That's useful for doing things like inviting new members (which is currently done through Aeryn), making sure that there are changelog entries for PRs (which this PR adds), or making sure that we remember to update Carthage files when Podspecs change (see example).

Currently, Peril only gets events for https://github.com/RxSwiftCommunity/contributors and you can see an example of what this looks like here: RxSwiftCommunity/contributors#35

So this PR adds the first danger rule for our community: making sure that changelog entries are present when non-testing code is changed (provided the repo contains a changelog at all).

This is a pretty big change for the org, so I wanted to flag this to @RxSwiftCommunity/contributors. I'm very happy to answer any questions or clarify any of the code below (it's a form of JavaScript, so if something doesn't make sense, please comment in-line). Once this PR gets merged, we can enable Peril org-wide.

Fixes RxSwiftCommunity/contributors#28.

…to changelog

freak4pc · 2017-12-31T09:53:49Z

Hey Ash, Thanks for this :)

I'm not entirely sure on forcing CHANGELOG changes on all repos, since most of the older Repos don't even hold a Change log as of today. So if we want to add this rule repo-wide, we would need to go back and fix some of the older repos (or even better do some sort of a cleanup of repos that are not maintained, but that's a much more aggressive effort).

Just a thought that came to mind. Anyways, really cool changes in general here :)

freak4pc · 2017-12-31T09:55:26Z

Just saw that you have a test and a rule for "it does nothing when there is no changelog file", so actually you can probably wipe everything I said :)

Should we add something like a #trivial flag to the body/title for changes that don't require a CHANGELOG entry ?

orta · 2017-12-31T13:31:44Z

Agree on #trivial (and include a note about it in the message)
If a repo doesn't have a CHANGELOG, then the rule won't fail, so don't worry about having it retroactively forcing it on smaller projects 👍

freak4pc · 2017-12-31T13:39:07Z

Thanks @orta
Yeah, I already mentioned I re-read the code and saw it doesn't do anything if the repo doesn't include a changelog to begin with :)

ashfurrow · 2017-12-31T16:02:26Z

Cool! Yeah I agree about #trivial and have added it in d1e77a5 .

freak4pc · 2017-12-31T16:20:30Z

pr.ts

-      warn("It looks like code was changed without adding anything to the Changelog.")
+      warn(
+        "It looks like code was changed without adding anything to the Changelog." +
+          "If this is a trivial PR that doesn't need a changelog, add #trivial to the PR title."


Just wondering, would a title change be enough for Peril to run again? or would you need CI to run again?

Or actually, I guess Peril doesn't even rely on a CI, it's its own step here, right?

Also, mega-super-uber-nit, there is an extra space on the second appended line (unless this is some TS convention I don't know 😛)

That's right – Peril should get webhook notifications for changed PR titles, but I'm not sure if it re-runs the danger file (I expect it does but couldn't confirm looking through Peril's code). @orta can you confirm?

It's fine if it doesn't re-run the Danger file, was just asking if Peril execute again when the title changes, or only when the PR is initially created. Seems like it's the former, which is great 👍

Any update to the PR, so a new body, a new commit, opening/closing, adding new assignees
etc - see http://artsy.github.io/blog/2017/09/04/Introducing-Peril/#Events

peril/settings-peril.json

Line 6 in d4dee01

"pull_request": "RxSwiftCommunity/peril@pr.ts",

freak4pc · 2017-12-31T16:21:08Z

pr.ts

    const files = [...danger.git.modified_files, ...danger.git.created_files]

    // Look for Swift files that aren't in a unit test directory.
-    const hasCodeChanges = files.find(file => file.match(/.*\.swift/) && !file.match(/(test|spec)/i))
+    const hasCodeChanges = files.find((file: any) => file.match(/.*\.swift/) && !file.match(/(test|spec)/i))


do we need the explicit any type here? Seemed to have worked before without it. Same for the hasChangelog definition on L29

Good question! This isn't necessary but was raising a TypeScript warning that file was being implicitly typed as any and that we should add a type specifier. It's very similar to some of the warnings you get in Swift.

Oh OK cool :)

In Swift you don't get warnings for inferred types, but I don't have enough (read- any) experience with TS unfortunately, so I take your word on that 😄

They don't come up very often, but I've seen warnings about implicit casts to Any in Swift (when working with Objective-C APIs, mostly).

freak4pc

A few thoughts but I really like this PR. Let's address these few last things and as far as I'm concerned, LGTM.

I don't think it's "a big change" because it would only affect repos that have a CHANGELOG which is great to enforce the uniformity we discussed in one of the threads here, so I'm all for.

If there are repos that already enforce this via Dangerfile, maybe those would need (or want) to remove that, so we don't get double warns

freak4pc · 2017-12-31T16:24:52Z

pr.ts

-  const hasChangelog = rootContents.data.find(file => changelogs.includes(file.name))
-  if (hasChangelog) {
+  const hasChangelog = rootContents.data.find((file: any) => changelogs.includes(file.name))
+  const markedTrivial = pr.title.match("#trivial")


Title only or body as well? (meaning either)

On Danger I have a rule for title or body and I usually find myself adding it in the body itself as well. I think it's a bit less intrusive... Adding "Flags" to the title kind dirties up the issues log

I actually prefer using the title because it gets included in the log (we use this at Artsy and when looking through commits, it's nice to immediately know that a PR bypassed rules like this one). But adding it to the body as well is pretty easy and I don't see any harm in it, so let's go ahead with that.

@ashfurrow This is just the PR title, I don't think it appears in the log anyways...

Anyways, if it doesn't have any negative effect or is too hard I personally prefer it. I prefer leaving room in the PR title as it's relatively short, and adding flags to the end of it would just have them truncated mostly (or not leave enough room for the actual PR title)

I agree – the RxSwiftCommunity has many ways of running our projects, and we should make this as applicable to contributors workflows as possible. I added this in 8276c81 .

freak4pc · 2017-12-31T16:27:46Z

pr.ts

-  if (hasChangelog) {
+  const hasChangelog = rootContents.data.find((file: any) => changelogs.includes(file.name))
+  const markedTrivial = pr.title.match("#trivial")
+  if (hasChangelog && !markedTrivial) {


Just realized something here @ashfurrow
@orta's suggestion was to warn (or just info if that exists) when there were changes and the issue is trivial.
Do you want to add that piece as well?

Ooooh yeah good idea!

freak4pc · 2017-12-31T18:20:04Z

pr.ts

+        markdown(baseMessage)
+      } else {
+        warn(baseMessage +
+            "If this is a trivial PR that doesn't need a changelog, add #trivial to the PR title."


.. to the PR title or body.

freak4pc · 2017-12-31T18:20:51Z

pr.ts

@@ -2,7 +2,7 @@ import { schedule, danger, warn, fail, markdown } from "danger"

 // Hey there!
 //
-// When a PR is opened, this file gets run. It's not straightforward, try to
+// When a PR is opened, this file gets run. It's notrueo


Whoopsie? :]

freak4pc · 2017-12-31T18:21:59Z

pr.ts

@@ -27,7 +27,7 @@ export const changelog = wrap("Require changelog entries on PRs with code change
  const rootContents: any = await danger.github.api.repos.getContent(getContentParams)

  const hasChangelog = rootContents.data.find((file: any) => changelogs.includes(file.name))
-  const markedTrivial = pr.title.match("#trivial")
+  const markedTrivial = [pr.title, pr.body].find(s => s.includes("#trivial"))


No biggie, but you could avoid the array (if you want) by doing:

(pr.title + pr.body).include?("#trivial")

(I actually stole this from danger.systems IIRC 😝 )

orta · 2017-12-31T18:48:01Z

pr.ts

-// TODO: Add a check to see if a changelog exists, then warn if it isn't in modified etc
+// Hey there!
+//
+// When a PR is opened, this file gets run. It's notrueo


freak4pc · 2017-12-31T19:08:16Z

pr.ts

-        warn(baseMessage +
-            "If this is a trivial PR that doesn't need a changelog, add #trivial to the PR title."
-        )
+        warn(baseMessage + "If this is a trivial PR that doesn't need a changelog, add #trivial to the PR title.")


This is still missing “or body” at the end.
Besides that LGTM!

Mmm oh yeah, thanks!

freak4pc · 2017-12-31T23:03:38Z

Given no objections or notes from @RxSwiftCommunity/contributors by tomorrow EOD, I'm gonna merge this in

ashfurrow · 2018-01-01T00:20:01Z

Sounds good 👍 Once merged, I'll do one more integration test before deploying across all our repos.

freak4pc · 2018-01-01T09:36:10Z

I had one tiny question about Peril. Does it also take care of running or taking into consideration the existing Dangerfile, or are these not related in any way and Danger runs by itself based on Repo/CI configuration ?

orta · 2018-01-01T15:22:26Z

It can run them, though you're using Ruby Dangerfiles everywhere, so it definitely won't run those

ashfurrow · 2018-01-01T16:06:13Z

Yeah, I figure as well roll out across the org, we can go through the existing repos using Danger and do the following:

Move any rules that should apply to organization-wide into Peril, and
Move any repo-specific rules into their own Dangerfiles stored in those repos (and specified in repos here.

There's no harm in running Danger on CI in addition to Peril – in fact, we use both at Artsy. Peril checks for PR metadata like missing changelogs or a PR that's not been assigned, whereas Danger looks at things more specific to the code within the PR and not the PR itself, like unit testing artefacts and code linting.

freak4pc · 2018-01-01T18:34:37Z

@ashfurrow Feel free to do the integration tests you were referring to. Thanks a lot for this! Really exciting :]

Adds changelog-checking rule.

99a7027

ashfurrow assigned orta Dec 31, 2017

Cleanup.

c2471f6

ashfurrow force-pushed the changelog branch from df2c79b to c2471f6 Compare December 31, 2017 05:10

Merge branch 'master' of https://github.com/RxSwiftCommunity/peril in…

ac60818

…to changelog

Ignores PRs marked as #trivial.

d1e77a5

freak4pc reviewed Dec 31, 2017

View reviewed changes

ashfurrow added 2 commits December 31, 2017 13:08

Sends a message on trivial PRs instead of a warning.

fd54e7f

Allows PRs to be marked as #trivial within their body text.

8276c81

freak4pc reviewed Dec 31, 2017

View reviewed changes

orta reviewed Dec 31, 2017

View reviewed changes

Cleanup from #1.

74bf3f9

freak4pc approved these changes Dec 31, 2017

View reviewed changes

Further cleanup for #1.

fbef13b

freak4pc merged commit 1d68df7 into master Jan 1, 2018

ashfurrow added a commit that referenced this pull request Jan 1, 2018

Adds logging to debug #1.

86ede54

ashfurrow added a commit that referenced this pull request Jan 1, 2018

More logging for #1, and an attempt to fix.

4465cec

ashfurrow added a commit that referenced this pull request Jan 1, 2018

Yet more logging for #1, and an attempt to fix.

e09b3c4

ashfurrow added a commit that referenced this pull request Jan 1, 2018

Undoes direct scheduling and adds more logging for #1.

9e13a1b

ashfurrow added a commit that referenced this pull request Jan 1, 2018

More logging for #1.

b671ef1

ashfurrow added a commit that referenced this pull request Jan 1, 2018

More logging for #1.

3e7ed5b

ashfurrow added a commit that referenced this pull request Jan 1, 2018

Removes plain closures, removes logging for #1.

23b4306

ashfurrow mentioned this pull request Jan 1, 2018

Peril Integration RxSwiftCommunity/contributors#28

Closed

ashfurrow deleted the changelog branch January 1, 2018 23:42

Changelog #1

Changelog #1

Uh oh!

Conversation

ashfurrow commented Dec 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

freak4pc commented Dec 31, 2017

Uh oh!

freak4pc commented Dec 31, 2017

Uh oh!

orta commented Dec 31, 2017

Uh oh!

freak4pc commented Dec 31, 2017

Uh oh!

ashfurrow commented Dec 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freak4pc Dec 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freak4pc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freak4pc Dec 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

freak4pc commented Dec 31, 2017

Uh oh!

ashfurrow commented Jan 1, 2018

Uh oh!

freak4pc commented Jan 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orta commented Jan 1, 2018

Uh oh!

ashfurrow commented Jan 1, 2018

Uh oh!

freak4pc commented Jan 1, 2018

Uh oh!

ashfurrow commented Dec 31, 2017 •

edited

Loading

freak4pc Dec 31, 2017 •

edited

Loading

freak4pc left a comment •

edited

Loading

freak4pc Dec 31, 2017 •

edited

Loading

freak4pc commented Jan 1, 2018 •

edited

Loading