Dangerfile exits early if build failed

[bvaughn]

No description provided.

[sophiebits]

Does this mean if danger succeeds then you push an update and break the build, the old comment sticks around?

[bvaughn]

Does this mean if danger succeeds then you push an update and break the build, the old comment sticks around?

I think the comment is only added/removed if output is logged. Given that we only log output if (a) the results JSON file is missing or (b) the size delta of a package is larger than some threshold– I think the danger comment will be added/updated/removed in the right cases.

[bvaughn]

Given the current changes, if a build fails in the scenario you describe, @sophiebits, this comment will replace the previous one:

If I just process.exit(0) without logging anything, I think the danger comment would just be removed (or not added to begin with). Which is preferable?

I guess maybe the latter, since you'd get a failed CI build at that point.

[sophiebits]

I think showing the message is preferable. It's harder to get misled that way.

[bvaughn]

Seems redundant, since there will also be an update from CI in that case:

In either case, the stale/invalid results stats would be removed.

[bvaughn]

I think it's better for the danger comment to be removed if the build fails. I think the red CI ❌ is enough feedback for that case.

Unless anyone objects, I think the behavior this PR currently exhibits is okay.

[trueadm]

This is much better than what we have now. Thanks for doing this @bvaughn!

[sophiebits]

ok, cool!

[bvaughn]

No problem 😄