kiegroup / git-backporting

Git backporting is a CLI tool to execute pull request backporting.
MIT License
16 stars 9 forks source link

fix: cherry-pick multiple commits in the correct order #115

Closed earl-warren closed 7 months ago

earl-warren commented 7 months ago

Fixes: https://github.com/kiegroup/git-backporting/issues/114

github-actions[bot] commented 7 months ago

Coverage report

St.:grey_question:
Category Percentage Covered / Total
🟒 Statements 90.84% 446/491
🟒 Branches 85.85% 176/205
🟒 Functions 88.33% 106/120
🟒 Lines 90.78% 433/477

Test suite run success

175 tests passing in 16 suites.

Report generated by πŸ§ͺjest coverage report action from e06c3a94950b98b2c9320df183dfe0ce5701a620

lampajr commented 7 months ago

Ok I think we have two options here:

  1. Change the commits order during the original pull request fetch (i.e., during the mapping) and then change all the tests as you already did
  2. Change the order in which the commits are applied (i.e., in the runner) and then no other changes would be required

NOTE: option 1 would require a change in the gitlab client as well

earl-warren commented 7 months ago

there's something I'm not sure about, in that with this fix (for github at least) the reverse is applied twice:

1. when mapping the original pr https://github.com/kiegroup/git-backporting/pull/115/files#diff-6e3804f9a1581e5155295a17bbbc71343fa375a15bf5383dd4c4049a44be09cdR64

2. when it has to scroll and cherry-pick the commits https://github.com/kiegroup/git-backporting/pull/115/files#diff-80854020f781d333495ba48208afccf09b44b58790aae94b10731f47b78e86ebR149

This means that at the end you are applying the commits in the same order as it was before πŸ€”

I need to check this a bit more

That's a leftover of a earlier attempt, my bad. And good catch.

earl-warren commented 7 months ago

Change the order in which the commits are applied (i.e., in the runner) and then no other changes would be required

That sounds better. I'll go with that now that I better understand where's what.

lampajr commented 7 months ago

Change the order in which the commits are applied (i.e., in the runner) and then no other changes would be required

That sounds better. I'll go with that now that I better understand where's what.

yeah I agree with you.

It could be useful to add something like this expect(GitCLIService.prototype.cherryPick).toHaveBeenLastCalledWith(cwd, "0404fb922ab75c3a8aecad5c97d9af388df04695", undefined, undefined) here https://github.com/kiegroup/git-backporting/blob/fe6be83074476d91c1b038fd7f03c4868e96f113/test/service/runner/cli-github-runner.test.ts#L683-L685

This way you can assure what is the last call of that method

earl-warren commented 7 months ago

Change the order in which the commits are applied (i.e., in the runner) and then no other changes would be required

That sounds better. I'll go with that now that I better understand where's what.

yeah I agree with you.

It could be useful to add something like this expect(GitCLIService.prototype.cherryPick).toHaveBeenLastCalledWith(cwd, "0404fb922ab75c3a8aecad5c97d9af388df04695", undefined, undefined) here

https://github.com/kiegroup/git-backporting/blob/fe6be83074476d91c1b038fd7f03c4868e96f113/test/service/runner/cli-github-runner.test.ts#L683-L685

This way you can assure what is the last call of that method

Thanks for the hint, exactly what I was looking for :heart:

earl-warren commented 7 months ago

Should be better now.