Skip to content

fix(git-node): do not assume release commit will conflict#871

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aduh95 merged 2 commits into from
Nov 19, 2024
Merged

fix(git-node): do not assume release commit will conflict #871

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a3b4442
fix(git-node): do not assume release commit will conflict
aduh95 Nov 16, 2024
5288297
add comment
aduh95 Nov 19, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 36 additions & 19 deletions lib/promote_release.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -147,23 +147,34 @@ export default class ReleasePromotion extends Session{
cli.warn(`Aborting release promotion for version ${version}`);
throw new Error('Aborted');
}
await this.cherryPickToDefaultBranch();

// Update `node_version.h`
await forceRunAsync('git', ['checkout', 'HEAD', '--', 'src/node_version.h'],
{ignoreFailure: false });
const appliedCleanly = await this.cherryPickToDefaultBranch();
Copy link
Member

@RafaelGSSRafaelGSSNov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
ContributorAuthor

@aduh95aduh95Nov 19, 2024•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@RafaelGSSRafaelGSSNov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@targostargosNov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
ContributorAuthor

@aduh95aduh95Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I decided to add comments


// Ensure `node_version.h`'s `NODE_VERSION_IS_RELEASE` bit is not updated
await forceRunAsync('git', ['checkout',
appliedCleanly
? 'HEAD^' // In the absence of conflict, the top of the remote branch is the commit before.
: 'HEAD', // In case of conflict, HEAD is still the top of the remove branch.
'--', 'src/node_version.h'],
{ignoreFailure: false });

// There will be remaining cherry-pick conflicts the Releaser will
// need to resolve, so confirm they've been resolved before
// proceeding with next steps.
cli.separator();
cli.info('Resolve the conflicts and commit the result');
cli.separator();
const didResolveConflicts = await cli.prompt(
'Finished resolving cherry-pick conflicts?',{defaultAnswer: true });
if (!didResolveConflicts){
cli.warn(`Aborting release promotion for version ${version}`);
throw new Error('Aborted');
if (appliedCleanly){
// There were no conflicts, we have to amend the commit to revert the
// `node_version.h` changes.
await forceRunAsync('git', ['commit', ...this.gpgSign, '--amend', '--no-edit', '-n'],
{ignoreFailure: false });
} else{
// There will be remaining cherry-pick conflicts the Releaser will
// need to resolve, so confirm they've been resolved before
// proceeding with next steps.
cli.separator();
cli.info('Resolve the conflicts and commit the result');
cli.separator();
const didResolveConflicts = await cli.prompt(
'Finished resolving cherry-pick conflicts?',{defaultAnswer: true });
if (!didResolveConflicts){
cli.warn(`Aborting release promotion for version ${version}`);
throw new Error('Aborted');
}
}

if (existsSync('.git/CHERRY_PICK_HEAD')){
Expand DownExpand Up@@ -488,8 +499,14 @@ export default class ReleasePromotion extends Session{

await this.tryResetBranch();

// There will be conflicts, we do not want to treat this as a failure.
await forceRunAsync('git', ['cherry-pick', ...this.gpgSign, releaseCommitSha],
{ignoreFailure: true });
// There might be conflicts, we do not want to treat this as a hard failure,
// but we want to retain that information.
try{
await forceRunAsync('git', ['cherry-pick', ...this.gpgSign, releaseCommitSha],
{ignoreFailure: false });
return true;
} catch{
return false;
}
}
}
Loading