fix(git-node): do not assume release commit will conflict#871
Uh oh!
There was an error while loading. Please reload this page.
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. |
| thrownewError('Aborted'); | ||
| } | ||
| awaitthis.cherryPickToDefaultBranch(); | ||
| constappliedCleanly=awaitthis.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
aduh95 merged commit ec6c6cb into nodejs:mainUh oh!
There was an error while loading. Please reload this page.
3 participants
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.