danger / danger-js

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

[BUG] danger.github.pr is undefined when in a mergequeue #1370

Closed airtonix closed 1 year ago

airtonix commented 1 year ago

Describe the bug

when danger js is run from a workflow triggered by a mergequeue:

"on":
  merge_group:
    types:
      - checks_requested

then:

danger.github.pr

will be undefined.

To Reproduce Steps to reproduce the behavior:

  1. have a github workflow with mergequeue event
  2. add in that workflow a dangerjs check that wants danger.github.pr.title
  3. enable mergequeue for in github branch protections for branch X
  4. create pr for branch Y into X
  5. add it to the merge queue
  6. witness explosion as danger.github.pr is not defined.

Expected behavior

The typescript types for danger.github.pr should be optional.

Screenshots If applicable, add screenshots to help explain your problem.

Your Environment

github

software version
danger.js "danger": "11.1.2",

| node | nodejs 16.17.0

| npm | pnpm 5.18.9

| Operating System | linux

Additional context Add any other context about the problem here.

orta commented 1 year ago

I don't know much about the merge queues but if it is effectively a way to act only on the pull request then I think it'd be fine to allow danger to generate the PR DSL when it is that action type. There's likely a declaration in the GitHub actions support about what types can generate it which you can add this to

( and I don't want .pr to be nullable because that's annoying for the 95% case of people using danger on PRs, but GA support allows running on any event with the event JSON in there, details should either be on the docs page of the changelog )

airtonix commented 1 year ago

well, the types should indicate reality: that pr will not always be there.

fbartho commented 1 year ago

@airtonix -- I think there's certain pragmatism necessary around our API types. It would be too inconvenient to make the common use-case(s) marked as optional, when in reality what we want to imply is "If PR should be there, then it is there".

If we follow your proposal to the logical conclusion, danger.github isn't or shouldn't always present, particularly when we're running on GitLab, for example.

That said, I'd certainly support better types around Merge Queues! This is a new feature from Github so it's obviously not defined in our types or tooling yet. We're considering adopting them in our repo, so I may take a look at that. If you have any ideas, feel free to write a Feature Request ticket.

(I've closed this ticket since the behavior is "as designed" and you're advocating some kind of behavior change that would be better discussed in a non-bug ticket)