danger / danger-js

⚠️ Stop saying "you forgot to …" in code review
http://danger.systems/js/
MIT License
5.24k stars 367 forks source link

Thoughts on Danger via GitHub Actions: beyond PRs #1048

Open mokagio opened 4 years ago

mokagio commented 4 years ago

TL;DR I might be doing something wrong, but I can get Danger to run "properly" in GitHub Actions workflows that are triggered by events other than PRs.

There's two possibilities:


I've been working on migrating our Peril setup to Danger + GitHub Actions on an off recently.

Danger works great for PR, but when it comes to other type of events that can trigger a GitHub Action workflow, it's not as effective.

For example, in this repo I set up a workflow to run on the status event and told it to post some messages.

No messages were posted. That's not that surprising, actually. How can Danger know that it's dealing with a status event of a commit that's part of a PR? Looking at the GitHubActions.ts CI source confirms my suspicion. In particular:

get useEventDSL() {
  return this.event.pull_request === undefined && this.event.issue === undefined
}

As far as I understand in the code, if useEventDSL() returns true, Danger will use the "simple DSL", which is... sort of empty?

// When there's an event we don't need any of ^
getPlatformReviewSimpleRepresentation: async () => ({}),

The Dangerfile I run in my test workflow dumps the danger object, and this is what it prints

{
    "git": {},
    "github": {
        "api": {
            "actions": {},
            "activity": {},
            "apps": {},
            "checks": {},
            "codesOfConduct": {},
            "emojis": {},
            "gists": {},
            "git": {},
            "gitignore": {},
            "interactions": {},
            "issues": {},
            "licenses": {},
            "log": {},
            "markdown": {},
            "meta": {},
            "migrations": {},
            "oauthAuthorizations": {},
            "orgs": {},
            "projects": {},
            "pulls": {},
            "rateLimit": {},
            "reactions": {},
            "repos": {},
            "search": {},
            "teams": {},
            "users": {}
        },
        "utils": {}
    },
    "utils": {}
}

Another example is the behavior with issues events, see #999.

Am I missing something? If so, then how I can get more information out of danger when running from an event that's not a PR. Otherwise, there's two options I see here:

orta commented 4 years ago

I added the random event support during the GH Actions alpha to give it a test run, and to be able to have some of my dangerfiles ported over from Peril and it mostly did what I wanted. This pre-dates GH actions running JS natively, so I was mostly just using Danger to give myself a consistent TS runtime with access to the GH API.

I think the github.api does work though, the output of dumping it might just have getters instead of hardcoded fields and so they may not show up

I'm open to it being more useful, for example github.event could be a thing which gives you the JSON dump, but I'm not sure about forcing every event to reach back to a PR at system level. A status is intentionally pretty decoupled from a PR by GH and you gotta put in work (via the search API) to find it (and it could return more than 1 PR for example) - so I'm not sure it's danger's responsibility to hook that up

f-meloni commented 4 years ago

When I did https://github.com/danger/danger-js/pull/940 my idea was exactly to extend Danger to all the PR related cases. I agree with Orta that status is not strictly a PR related event, more a commit related event, there could in fact not be a related PR. What about allowing some env variables in order to be able to still run Danger in Actions on those cases? Then doesn't matter what the actual event is, if is not a PR related one and you can retrieve the actual PR data you can still pass that to Danger

mokagio commented 4 years ago

Thanks @orta and @f-meloni for the feedback. 😄 🙏

I think the github.api does work though, the output of dumping it might just have getters instead of hardcoded fields and so they may not show up

I didn't think of that 😳 . I'm a JS/TS noob.

I'm not sure about forcing every event to reach back to a PR at system level. A status is intentionally pretty decoupled from a PR by GH and you gotta put in work (via the search API) to find it (and it could return more than 1 PR for example) - so I'm not sure it's danger's responsibility to hook that up

+1. I really like the "Stop saying 'you forgot to …' in code review" tagline. Trying to make too much of a general purpose tool would be a slippery slope of complexity

What about allowing some env variables in order to be able to still run Danger in Actions on those cases? Then doesn't matter what the actual event is, if is not a PR related one and you can retrieve the actual PR data you can still pass that to Danger

That sounds like a great place to start. It might be there's more that can be done in the current setup, too, and it's just that I don't know how to read some of those information. I'll play around with that a bit.