fix(git-node): do not assume release commit will conflict #871
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #871 +/- ##
==========================================
- Coverage 83.08% 80.08% -3.00%
==========================================
Files 37 39 +2
Lines 4251 4676 +425
==========================================
+ Hits 3532 3745 +213
- Misses 719 931 +212 ☔ View full report in Codecov by Sentry. |
| @@ -147,23 +147,31 @@ export default class ReleasePromotion extends Session { | |||
| cli.warn(`Aborting release promotion for version ${version}`); | |||
| throw new Error('Aborted'); | |||
| } | |||
| await this.cherryPickToDefaultBranch(); | |||
| const appliedCleanly = await this.cherryPickToDefaultBranch(); | |||
There was a problem hiding this comment.
I think it's better to check if the release is semver-patch instead of appliedCleanly. It will be confusing to people when reading the code.
There was a problem hiding this comment.
I don't think it's a good idea to assume that a semver-patch release never conflicts, and that a non-semver-patch would always conflict. I don't understand why it would be confusing, on the contrary it seems to me having fewer assumptions in code leads to fewer confusion in my experience.
There was a problem hiding this comment.
I feel confusing the git checkout HEAD^ | git checkout HEAD part considering a appliedCleanly variable. Maybe a comment? I'm fine leaving it as appliedCleanly with a comment on that part
There was a problem hiding this comment.
We could use git rev-parse HEAD before trying to cherry-pick, and then git checkout (or the less ambiguous git restore) with the exact commit sha.
There was a problem hiding this comment.
I decided to add comments
As I was testing
git node release --promotewith nodejs/node#55879, I realized the assumption that a release commit would necessarily conflict when cherry-picked to the default branch is wrong for the case of semver-patch releases.