feat(#181): reconstruct script in failure comment

[oliverpool]

fixes #181

This is working, but I am not satisfied with the code quality... Any feedback on how to improve the code would be nice!

Description

This adds a reconstruction of the attempted steps in the failure comment, to allow users to reproduce the failure (and address the conflicts), for instance:

The backport to v3 failed. Check the latest run for more details.

Reconstruction of the attempted steps (beware that escaping may be missing):

git fetch origin v3
git switch -c custom-failure-head-v3 origin/v3
git fetch origin pull/2368/head:pr/2368
git cherry-pick -m 1 --strategy=recursive --strategy-option=theirs 28f63db774185f4ec4b57cd9aaeb12dbfb4c9ecc
git push origin custom-failure-head-v3
# the step below failed
# github.createPullRequest

How Has This Been Tested?

Tests were already present, they have been adjusted.

Checklist

• Tests added if applicable.
• Documentation updated if applicable (I don't think that this is applicable).

Merge criteria:

• The commits and have meaningful messages; the author will squash them after approval or will ask to merge with squash.
• Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
• The developer has manually tested the changes and verified that the changes work

I am not sure I can easily test this change manually.

[github-actions]

Coverage report

St.❔	Category	Percentage	Covered / Total
🟢	Statements	89.37% (-0% 🔻)	555/621
🟢	Branches	85.48% (+0.06% 🔼)	259/303
🟢	Functions	87.92% (+0.32% 🔼)	131/149
🟢	Lines	89.22% (+0.01% 🔼)	538/603

Show files with reduced coverage 🔻

St.❔	File	Statements	Branches	Functions	Lines
🟢	runner/runner.ts	89.74% (-0.26% 🔻)	95% (-5% 🔻)	88% (+8% 🔼)	89.74% (-0.26% 🔻)

Test suite run success

218 tests passing in 18 suites.

Report generated by 🧪jest coverage report action from 15b716d

[lampajr]

Hello @oliverpool , amazing work - thanks a lot!!

I will try to take a look at this as soon as I can 🙏

[lampajr]

Overall looks good, seems working as expected but I agree that it is a bit complex and hard to read (I am thinking more on the maintenance side of things).

What about having something a bit easier, e.g., update the GitCLIService so that each method takes a new input (a list of strings provided by the caller) where the method will add the command being executed:

  // trace can also be a list
  async createLocalBranch(cwd: string, newBranch: string, trace: string[]): Promise<void> {
    this.logger.info(`Creating branch ${newBranch}`);
    trace.push("...") // here you can add the git command being execute
    await this.git(cwd).checkoutLocalBranch(newBranch);
  }

This way the caller has the full trace of the commits being executed w/o having to re-run the steps with a fake implementation. This way you don't even need the Generator and etc. I guess.

The caller will change "trace" object for every PR being backported, so that each backporting will have its own tracing object (list of strings)

Additionally I would also enable this "tracing" information in the PR comment only when providing an additional CLI/GHA parameter, so that we are still backward compatible if others don't need this new feature. wdyt?

[oliverpool]

Thanks for the review!

update the GitCLIService so that each method takes a new input (a list of strings provided by the caller) where the method will add the command being executed

The only drawback that I see, is that it would make it harder to include the later (non-attempted) steps in the comment. For this, each step should:

1. add to the trace
2. if a previous step failed, return (eventually a value)
3. otherwise attempt the actual step
4. on actual step failure, set the "previousStepFailure", to ensure that the following steps still add to the trace

  // trace can also be a list
  async createLocalBranch(cwd: string, newBranch: string, trace: string[]): Promise<void> {
    trace.push("...") // here you can add the git command being execute
    if(this.previousStepFailure){
      return;
    }
    this.logger.info(`Creating branch ${newBranch}`);
    try{
      await this.git(cwd).checkoutLocalBranch(newBranch);
    } catch(e) {
      this.previousStepFailure=e;
    }
  }

I have updated the tests in 15b716d to show that later steps are part of the comment (even if not attempted).

Additionally I would also enable this "tracing" information in the PR comment only when providing an additional CLI/GHA parameter, so that we are still backward compatible if others don't need this new feature. wdyt?

As a user, I don't see any drawback in having this comment by default. On the contrary, it will spare me much time: it can spare me to have to look at the log if I see the failure was caused by a conflict (likely 99% of our failures) and make it quicker to solve locally.
If the content of the comment is part of the compatibility promise, then sure, there should be a now flag; set to false by default.

[lampajr]

Alright, I see your point and I was not considering the later steps after the one failed, but I agree they will be useful if anyone wants to complete the backporting manually and there are multiple cherry picks.

With regard the additional flag, yeah we can side this as an improvement of the existing error message.

Therefore I believe we are ready to go, we could improve it later if we are not satisfied by the implementation.

FWIW I did a simple test here: lampajr/backporting-example#41 (comment)

Well done @oliverpool and thank you very much for the improvement!! ❤️