Skip to content
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

fix release proposal failing when stderr has output #5362

Merged
merged 1 commit into from
Mar 5, 2025
Merged

fix release proposal failing when stderr has output #5362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
30 changes: 8 additions & 22 deletions scripts/release/helpers/terminal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,8 @@ let timer
let current

// Output a command to the terminal and execute it.
function run (cmd, treatStderrAsFailure = true) {
capture(cmd, treatStderrAsFailure)
function run (cmd) {
capture(cmd)
}

// Ask a question in terminal and return the response.
Expand Down Expand Up @@ -55,7 +55,7 @@ function checkpoint (question) {
}

// Run a command and capture its output to return it to the caller.
function capture (cmd, treatStderrAsFailure = true) {
function capture (cmd) {
if (flags.debug) {
log(`${GRAY}> ${cmd}${RESET}`)
}
Expand All @@ -76,25 +76,11 @@ function capture (cmd, treatStderrAsFailure = true) {

if (flags.debug) {
log(stdout)
if (stderr) {
if (flags['no-abort-on-error']) {
log(`${RED}${stderr}${RESET}`)
} else {
fatal(
stderr,
'Aborting due to error! Use --no-abort-on-error to ignore and continue.'
)
}
}
} else if (treatStderrAsFailure && stderr) {
if (flags['no-abort-on-error']) {
log(`${RED}${stderr}${RESET}`)
} else {
fatal(
stderr,
'Aborting due to error! Use --no-abort-on-error to ignore and continue.'
)
}
Comment on lines -79 to -97
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing this will unfortunately get us back to the original problem, which prompted me to make this change in the first place: The branch-diff tool will exit with a status code zero, but output errors on STDERR if there's a permission issue.

While I have also updated the requirements check to check that the expected permissions are in place, it would still be nice to have some sort of guardrail in place for when branch-diff runs into issues.

However, instead of defaulting to throwing on output to STDERR, maybe we should change the default to ignoring it, and only for branch-diff commands we can enable it.

Alternatively, or in combination with this, we could also change the default to only output STDERR, but not abort on it.

What do you think about all this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Any way to force the command to throw when there is an error at the command level? A bit like when the output if redirected to /dev/null, but kind of the opposite of that.

Copy link
Member Author

Choose a reason for hiding this comment

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

For now I will merge and we can address in another PR since this is blocking all releases.

log(`${RED}${stderr}${RESET}`)
}

if (result.status) {
throw new Error(stderr)
Comment on lines +82 to +83
Copy link
Collaborator

Choose a reason for hiding this comment

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

If result.status is not zero, and error will already have been thrown above on line 71, so this if-block can be removed.

}

return stdout
Expand Down
6 changes: 3 additions & 3 deletions scripts/release/proposal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ try {

try {
// Pull latest changes in case the release was started by someone else.
run(`git remote show origin | grep v${newVersion} && git pull --ff-only`, false)
run(`git remote show origin | grep v${newVersion} && git pull --ff-only`)
} catch (e) {
// Either there is no remote to pull from or the local and remote branches
// have diverged. In both cases we ignore the error and will just use our
Expand Down Expand Up @@ -113,7 +113,7 @@ try {

// Cherry pick all new commits to the proposal branch.
try {
run(`echo "${proposalDiff}" | xargs git cherry-pick`, false)
run(`echo "${proposalDiff}" | xargs git cherry-pick`)

pass()
} catch (err) {
Expand Down Expand Up @@ -147,7 +147,7 @@ try {

// Create or edit the PR. This will also automatically output a link to the PR.
try {
run(`gh pr create -d -B v${releaseLine}.x -t "v${newVersion} proposal" -F ${notesFile}`, false)
run(`gh pr create -d -B v${releaseLine}.x -t "v${newVersion} proposal" -F ${notesFile}`)
} catch (e) {
// PR already exists so update instead.
// TODO: Keep existing non-release-notes PR description if there is one.
Expand Down
Loading