Do not invalidate signatures on clean merge #324
Conversation
Setting this parameter to true tells pulldasher to not invalidate empty merge commits. We do this by grabing the file changes for merge commits and then reducing to the newest valid commit.
It is possible for a pull request to only contain clean merge commits. We can handle this case gracefully by falling back to the pull request's creation date.
| } | ||
| })) | ||
| }) | ||
| .then(expandedCommits => { |
There was a problem hiding this comment.
At this point, we have the all the commits for the whole pull, But the order seems unspecified. The API doesn't say anything about the order of commits, but I don't think there's anything we can intrinsically order them by. It seems like the rest of the code expects them to be in oldest-first order.
There was a problem hiding this comment.
I believe I read on a post somewhere that they were chronologically ordered by commit date by default, but it definitely does not guarantee it in the api documentation. I will add a change to explicitly check this.
Here is a blog post about it, but we shouldn't rely on that behavior.
|
This looks good! So, CR 👍 on this part, but it's only a partial solution. This code change covers us when refreshing the pull (I think). When we get notified via a webhook that a new commit is pushed, we immediately invalidate the signatures: pulldasher/controllers/githubHooks.js Lines 65 to 69 in 5f07ecb We could solve this systematically (harder) by not storing dev_block 👍 on this being a partial solution at the moment. |
Currently the github REST api returns commits in chronological order by commit date, but I don't see this on the specification for the endpoint. Let's not rely on this behavior.
This function needs to convert the string format to a date object to do what we want.
This was some functionality that we needed for our instance of pulldasher, so I added a quick solution.
One peculiarity is, since selecting the current changes during a conflict is an empty change, some conflicts resolutions are considered clean.
Thought I would open a pull here in case it could be useful for solving #239 on your end.
With `ignoreCleanMerge` set to `true`
With `ignoreCleanMerge` set to `false`
Ref: #239