PR is not exists on the release notes

[wellDan28]

Hey,
I've facing some unexpected behavior:
In my repo the tag v1.13.0 was added after a pull-request was merge to the master but the pull-request is not include in the release notes.
I'm running the following command:
gren r --token="XXXXXXXXXXX" --data-source=prs --tags="v1.13.0..v1.10.0"

Demo repo:
https://github.com/wellDan28/gren-test/releases

Is this is the expected behavior in this scenario?

[alexcanessa]

Hello @wellDan28

Let me investigate and I'll come back to you.

[svanzoest]

I am seeing this as well, where the last commit does not seem to be accounted for.

[phun-ky]

I am also seeing this, the commits from a merged PR is ommited. Is it because of fast forward or something? I see the desired commits with git log though..

[alexcanessa]

Ok, looks like this is a top priority.

Working on this next 👍

[lukemartin]

I've been looking into fixing this, and have a hit a bit of a roadblock.

I think the main problem is down to the time difference between a pull request being closed and the actual merge commit created.

For example:

$ git log

commit 754bebe07899617d4b4525bfa9b8596b4e9c8148 (HEAD -> master, tag: v1, origin/master, origin/HEAD)
Author: Luke Martin <luke@lukemartin.me>
Date:   Fri Mar 2 22:48:59 2018 +0000

	Update README.md (#2)

commit 85fbf8806afc57d245f5bf7f1b923e14f3c3e166
Author: Luke Martin <luke@lukemartin.me>
Date:   Fri Mar 2 22:48:28 2018 +0000

	Update README.md (#1)

When creating a release:

gren release --data-source=prs

The output only contains details for PR #1

Here we can see the merge commit 754bebe was created at 22:48:59. But, when inspecting the metadata for the missing PR:

{
	...
	id: 301929932,
	number: 2,
	title: "pr2",
	state: "closed",
	created_at: "2018-03-02T22:48:54Z",
	updated_at: "2018-03-02T23:10:46Z",
	closed_at: "2018-03-02T22:49:00Z",
	body: "pr2",
	...
}

The PR is actually marked as closed 1 second later, at 22:49:00. Therefore gren thinks the PR was closed after the tag was created, but we know it wasn't.

We can't even use a combination of state and updated_at because post-merge comments will update the value making it unreliable.

I've poked around the GitHub API docs and I can't find anything that might help here. @alexcanessa any ideas? I'm happy to attempt a fix if you can push me in the right direction.

Cheers! :)

[lukemartin]

I've been thinking about this a bit more, and was wondering if using commit id's instead of dates might be more reliable for this sort of thing?

exxxxxxx (tag3)
dxxxxxxx
cxxxxxxx
bxxxxxxx (tag2)
axxxxxxx (tag1)

// release tag3
// commits in release must be `cxxxxxxx,dxxxxxxx,exxxxxxx`
// look for all closed PRs which have a merge commit id which is one of `cxxxxxxx,dxxxxxxx,exxxxxxx`
// we can get this from `listIssueEvents`

[wellDan28]

I'm thinking that adding a second to each of PR merged time fields might be a sufficient solution (not clean but sufficient)

[wellDan28]

@alexcanessa what do you think about 👆 ?

[alexcanessa]

@wellDan28 not totally sure about the "adding a second" solution as we don't know if there is a set time or a delay caused by their server etc.
In fact, the data is different for the last to PRs merged in this repo.

@lukemartin looking for all commits is quite an expensive solution in a real scenario when you have lots of commits.

Possible solution

Although there are a few properties that contain data that we might use.
Looking at two PRs for this repo:

Pull Request #157

{
    "number": 157,
    "created_at": "2018-04-03T05:47:03Z",
    "updated_at": "2018-04-03T08:43:57Z",
    "closed_at": "2018-04-03T08:43:57Z",
    "merged_at": "2018-04-03T08:43:56Z",
    "merge_commit_sha": "78786b31a7a85afc884092af41c681669fe9652e"
}

Pull Request #148

{
    "number": 148,
    "created_at": "2018-03-08T11:00:44Z",
    "updated_at": "2018-04-19T09:56:18Z",
    "closed_at": "2018-04-19T09:54:59Z",
    "merged_at": "2018-04-19T09:54:59Z",
    "merge_commit_sha": "9ff8e2a191ff5ed3b900749f7a3a059f240ad646",
}

I can see that:

• In #157: closed_at is one second after merged_at
• In #148: closed_at is the same time as merged_at

Maybe one of the two properties (closed_at I would assume) is what we're actually looking for.

The commit SHA

In case this isn't enough, there is a merge_commit_sha, that might give us the actual time.

This one is the commit for #148:

{
  "sha": "9ff8e2a191ff5ed3b900749f7a3a059f240ad646",
  "commit": {
    "author": {
      "name": "Alexander Vassbotn Røyne-Helgesen",
      "email": "XXXXXXXXXXXXXXXXX",
      "date": "2018-04-19T09:54:58Z"
    }
}

Now, looks like commit.date ("2018-04-19T09:54:58Z") is even one second less than both merged_at and closed_at. (which makes me think that the commit solution won't work)

---

I think we should look into those properties and see what is a right reflection in terms of timing.

[shaikatz]

Hi any news about that?
I've just experienced it in Kamus

[Tybot204]

+1 to using closed_at as the date check. My current theory is that merging triggers a PR to also be closed after, and that they are not the same state transition in GitHub. I've noticed on merging some PRs that the merge commit is made immediately, but the page hangs and doesn't actually close out the PR for a moment.

[Tybot204]

@alexcanessa I'm doing a bit of research into the PR attributes returned from GitHub. It seems to me we want to use closed_at for sure, but need to continue to filter out branches that don't have a merged_at (as they may have been closed without merging).

I found where the "merged vs. just closed" check is happening in Gren, but I don't think I'm following along on how the times are checked. If you could point me in the right direction there, I'd be happy to throw up a PR to discuss further!

[hiyangw]

+1 closed_at
I got same issue when I create a new release tag if I updated PR later. Then I am not able to see latest PR on list.

"created_at": "2020-04-27T23:52:15Z",
"updated_at": "2020-04-28T22:36:55Z",
"closed_at": "2020-04-28T02:11:13Z",
"merged_at": "2020-04-28T02:11:13Z"

[alexcanessa]

@hiyangw this should be part of the latest release.