arielgreen commented 3 years ago

If you haven’t already, check out our contributing guidelines for onboarding!

Problem

As part of our new Expensify.cash Deploy & QA process, we need a way to be able to figure out the set of PRs that moved the application from some older version to some newer version.

Why is this important?

This operation has myriad usages in the Deploy & QA process. One such use is to gather the PRs that have been deployed to staging since the previously QA'd version, so that Applause can QA all those PRs.

Solution

Create a Javascript function in expensify-common called getPullRequestsMergedBetween that will take in two git refs, and will gather a list of PR numbers of all PRs merged between those two refs by:

1) Executing the following shell command to list all commit messages between the two refs:

`git log --format="%s" ${FROM_REF}...${TO_REF}`

2) Apply this regex to parse the PR numbers of merged PRs from the results:

`/Merge pull request #(\d{1,6})/g`

3) Return the resulting list of PR numbers.

To be considered complete

Document manual testing done
Provide automated unit tests for this function

Internal issue Upwork listing

tugbadogan commented 3 years ago

Hi @arielgreen

I would like to work on this issue. Here is my proposal:

I'm planning to create getPullRequestsMergedBetween function that will take 2 git refs as parameters in GithubUtils class.
I will execute git log --format="%s" ${FROM_REF}...${TO_REF} command via exec function like this https://github.com/Expensify/Expensify.cash/blob/0421229ce750290d181c2e150cf97a2640c59c37/.github/actions/bumpVersion/bumpVersion.js#L17
Then, I will search this regex pattern on result with something like this new RegExp(/Merge pull request #(\d{1,6})/, 'g');
Finally, I will add matched PR numbers to list and return it.

A question for you, in which context are we going to call this function? Are we going to get the repository folder as a parameter or are we going to have something like octokit client?

roryabraham commented 3 years ago

@tugbadogan Your proposal looks good. My only comment is that I'd like to see automated unit tests for some of the code (at least for the regex).

A question for you, in which context are we going to call this function? Are we going to get the repository folder as a parameter or are we going to have something like octokit client?

This is a great question. This function will be called from the context of a Javascript Github Action. Because this code will be executed in a remote Github Actions Runner, the git log command will be executed from the context of that runner. Does that answer your question?

roryabraham commented 3 years ago

Oh, and I'm also in the process of promisifying our exec calls, so you should actually use exec like this:

const {promisify} = require('util');
const exec = promisify(require('child_process').exec);
exec(yourCommand)
    .then(({stdout, stderr}) => {
        // Success callback
    })
    .catch((err) => {
        // Error callback
    });

Also note that we don't use async/await anywhere in our JS code to maintain consistency.

arielgreen commented 3 years ago

@tugbadogan I sent over an offer via Upwork. Once you accept that, I'll assign you the issue and you are good to proceed with your PR.

tugbadogan commented 3 years ago

@tugbadogan Your proposal looks good. My only comment is that I'd like to see automated unit tests for some of the code (at least for the regex).

I think unit tests are in __tests__ directory. Sure, I can add unit test for this function. https://github.com/tugbadogan/expensify-common/tree/master/__tests__

This is a great question. This function will be called from the context of a Javascript Github Action. Because this code will be executed in a remote Github Actions Runner, the git log command will be executed from the context of that runner. Does that answer your question?

Yes that's helpful. I also want to ask is there an easy way to test this function from Github Action?

Oh, and I'm also in the process of promisifying our exec calls, so you should actually use exec like this:
const {promisify} = require('util');
const exec = promisify(require('child_process').exec);
exec(yourCommand)
    .then(({stdout, stderr}) => {
        // Success callback
    })
    .catch((err) => {
        // Error callback
    });
Also note that we don't use async/await anywhere in our JS code to maintain consistency.

OK. No problem. So my function will return a promise, right?

roryabraham commented 3 years ago

OK. No problem. So my function will return a promise, right?

Yep, that's correct.

I also want to ask is there an easy way to test this function from Github Action?

Yes, you should be able to test this from a Github Action by creating a test workflow that checks out the expensify-common repo w/ the commit ref you want to test. From there, you can run your function using a simple node script and we can see if it works as expected. I can help you with that when you're ready, but the test workflow might look something like this: https://gist.github.com/roryabraham/c6cca86488b915c46bfecb4d6f1edfa7

tugbadogan commented 3 years ago

Thanks @roryabraham that's very helpful! I'll send the PR soon.

roryabraham commented 3 years ago

@tugbadogan Any update here?

Also, please note that we can't use async/await anywhere in our Javascript code. You must use promises instead. Thanks!

tugbadogan commented 3 years ago

HI @roryabraham , I implemented the function and submitted the PR. I tried to test it with GitHub Actions as you recommended, however it didn't work for some reason.

https://github.com/tugbadogan/expensify-common/runs/2046888897

I'm getting the following exception when the function is executed.

{
Error: Command failed: git log --format="%s" 9254b614399175a6a85745a99a6fb5bb9789915d...07bc78b5932dd94e2576227d5dc89b6017877186
fatal: Invalid symmetric difference expression 9254b614399175a6a85745a99a6fb5bb9789915d...07bc78b5932dd94e2576227d5dc89b6017877186

    at ChildProcess.exithandler (child_process.js:294:12)
    at ChildProcess.emit (events.js:198:13)
    at maybeClose (internal/child_process.js:982:16)
    at Socket.stream.socket.on (internal/child_process.js:389:11)
    at Socket.emit (events.js:198:13)
    at Pipe._handle.close (net.js:607:12)
  killed: false,
  code: 128,
  signal: null,
  cmd:
   'git log --format="%s" 9254b614399175a6a85745a99a6fb5bb9789915d...07bc78b5932dd94e2576227d5dc89b6017877186',
  stdout: '',
  stderr:
   'fatal: Invalid symmetric difference expression 9254b614399175a6a85745a99a6fb5bb9789915d...07bc78b5932dd94e2576227d5dc89b6017877186\n' }
Post Run actions/checkout@v2

Do you know why it works locally, but it doesn't work in GitHub Actions context?

roryabraham commented 3 years ago

Hi @tugbadogan, thanks for submitting that PR. From what I understand, that error basically means that one or both of the commit hashes you used in the git log command doesn't exist. I think the reason you're seeing that error is because the Github Runner (the cloud-hosted machine that executes your Github Action) doesn't have the complete Git history. If memory serves, actions/checkout only checks out the main or master branch by default.

In order to fetch the full history for a git repo, you could either execute a git fetch --all command or, more simply, modify your usage of the actions/checkout@v2 action like so:

- uses: actions/checkout@v2
  with:
    fetch-depth: 0

tugbadogan commented 3 years ago

Hi @roryabraham, yes that solved the problem. Thanks!

The output of sample GitHub Action that uses this module: https://github.com/tugbadogan/expensify-common/pull/8/checks?check_run_id=2051334450

Here's the latest version of manual testing script and GitHub action https://github.com/tugbadogan/expensify-common/blob/master/testGitUtils.js https://github.com/tugbadogan/expensify-common/blob/master/.github/workflows/testGitUtils.yml

The only issue is that it didn't work in the beginning and I had to use node v12 to make it work. I was getting the following error message when I use node v10. I found that matchAll function is not supported before version 12 https://node.green/#ES2020-features-String-prototype-matchAll-basic-functionality

TypeError: stdout.matchAll(...) is not a function or its return value is not iterable
5
    at exec.then (/home/runner/work/expensify-common/expensify-common/lib/GitUtils.jsx:16:55)

roryabraham commented 3 years ago

Okay, thanks for the updated code, testing, and heads-up about the required node version. Looks good, PR is merged!

tugbadogan commented 3 years ago

Thank you for your help @roryabraham!

tugbadogan commented 3 years ago

@arielgreen @roryabraham Is there anything else to do here? Shall we close the issue?

roryabraham commented 3 years ago

@tugbadogan nothing else to do here, so I'll close this issue. There's a standard 7-day wait period between the time your PR is merged and when we pay out issues, to allow some time to see if your code introduces any unforeseen regressions.

Expensify / App

[GH Actions] Create getPullRequestsMergedBetween function #1576

Problem

Why is this important?

Solution

To be considered complete