-
Notifications
You must be signed in to change notification settings - Fork 3
fix: reject when command fails. perform commit with --no-verify #40
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.
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
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThe version update script's error handling has been refined by modifying the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (2)
Comment |
| const execPromise = (command) => | ||
| new Promise((resolve, reject) => exec(command, (e, out) => (e && reject(e)) || resolve(out))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to use:
const execPromise = promisify(exec)
Like in https://github.com/Sofie-Automation/sofie-package-manager/blob/main/scripts/test-everything.js#L6 and some other places?
| } | ||
|
|
||
| await execPromise(`git commit -m "chore(release): v${nextVersion}"`) | ||
| await execPromise(`git commit --no-verify -m "chore(release): v${nextVersion}"`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not keen on this - I think if there is a pre-commit that fails, something weird is going on and we shouldn't proceed with making a new version until it's fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eh... this is only commiting a readme and the package.json so the precommit isn't all that relevant imo
About the Contributor
This pull request is posted on behalf of myself.
Type of Contribution
This is a: Bug fix
Current Behavior
While making a release of a library, I was confused that it didnt make a commit. I then discovered it had made a tag, based on the existing commit.
This is because husky is misconfigured and is causing the commit to fail.
New Behavior
This ensures that when a commaand fails, it will cause a rejection and result in an error.
Skip running any commit hooks when making the commit, as they shouldn't matter and cause unexpected failures in this simple command
Testing Instructions
Other Information
Status
Summary
This PR fixes two critical issues in the version update automation script:
Error Handling Fix
The
execPromisewrapper function was modified to properly reject promises on command failures. Previously, the function would resolve with the error object instead of rejecting, causing error conditions to be silently passed through as successful resolutions. This is now corrected to use standard Promise semantics:reject(e)on errors andresolve(out)on successful output.Commit Hook Bypass
Added the
--no-verifyflag to the git commit command that creates the release commit. This prevents git hooks (such as husky) from running during the automatic versioning commit, which could cause unexpected failures and prevent the commit from being created. This ensures that the version bump and corresponding tag are always created together, resolving an issue where tags could be created without corresponding commits due to hook misconfiguration.Formatting
The code properly uses tabs for indentation as per project standards.