feat: add support for workflow_dispatch event

[jhoffmcd]

Closes #45

• Add support and check for the workflow_dispatch event
• Add a new optional input for pr-number
• In the case of a workflow_dispatch event, fetches data from GitHub for the PR that provides the relevant data as pr.

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[simoneb]

See inline comments. Add tests

[simoneb]

this may throw an exception if the pr-number input is not provided and this is not run in the context of a pull request. make it more robust

[jhoffmcd]

I've added a check to log an error if a PR number isn't present.

Looks like core.getInput will return as an empty string for an optional input, so this should be okay?

https://github.com/actions/toolkit/blob/2f164000dcd42fb08287824a3bc3030dbed33687/packages/core/src/core.ts#L69-L77

[simoneb]

this is not the problem. if PR_NUMER is falsy and github.context.payload.pull_request is falsy, it will throw a JavaScript error

[jhoffmcd]

I've included a check after this statement since a pull request is required to do any of the later work. Is that not enough? I can add a check before I do the variable assignment to pullRequestNumber if that is better?

const pullRequestNumber = PR_NUMBER || pr.number

if (!pullRequestNumber) {
  return logError(
    'No pull request number has been found. Please make sure a pull request number has been provided'
  )
}

[jhoffmcd]

I suppose I can introduce a variable hasPullRequest? Then I can check the value of that, and error out if nothing is present or defined.

const hasPullRequest = pull_request || PR_NUMBER

[simoneb]

The action needs to be robust enough that wrong user input doesn't lead to unexpected errors

[jhoffmcd]

@simoneb okay check it out now, I think there are enough checks and conditionals to prevent an issue based on user input or context.

[jhoffmcd]

I'm not quite sure what should be added as a test here since there is not much to test in getPullRequest, it's mostly just a wrapper for octokit. Definitely will try to write some tests if I can understand more of what we could effectively test here.

[simoneb]

including @gilach

[simoneb]

The logic looks good but the structure of the code needs improvement. See inline comments.

[simoneb]

these boolean variables are unnecessary. since you're checking for their truthyness you don't need temp variables

[simoneb]

extract this logic in a function, it will avoid cluttering the body of the main function and it will spare the need for mutable state. just return a "pull request" from that function and you can then extract the number from it

[jhoffmcd]

@simoneb I've made some changes that I think encapsulate the functionality pretty nicely. I looked into writing some tests and found that the current way I wrote this would require a lot of mocking to get the test to work. I can do that, or I can pass the inputs, github object, etc as arguments to getPullRequest which would make it a little bit easier to test. What path do we prefer for our tests here?

[simoneb]

I'm still hoping that we can achieve a better implementation. The code works, but it can be written in a better way:

• the logic to test if we are in a supported context is too specific. We don't really care if we are in a workflow dispatch event, as long as we have a PR_NUMER in case we are not in a pull_request context
• let's get the inputs in a single place, meaning that you can move loading the PR_NUMER in the index.js file
• let's keep the logic to use payload.pull_request outside of the getPullRequest function. That function should not load any globals but it should get all the inputs it needs as arguments, so it's more easily testable. This includes the PR_NUMBER argument
• in index.js the logic to obtain a pull request object can be simplified to:

const pr = pull_request || await getPullRequest(PR_NUMER)

[jhoffmcd]

@simoneb I've made the requested changes.

I did try and write a test for this new function, but it ends up just testing whether or not we return the mocked value from mocking @actions/github for the octokit stuff. Not sure how valuable a test like that is.

[jhoffmcd]

@simoneb Yeah good call-outs. I think we can leave it as pr-number according to the docs: https://docs.github.com/en/actions/creating-actions/metadata-syntax-for-github-actions#inputsinput_id.

Looks like - is accepted as part of the string identifier for an input. I've committed your suggestions.