-
Notifications
You must be signed in to change notification settings - Fork 2
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
More graceful handling of lacking merge/close permissions #20
Conversation
src/process.ts
Outdated
return { | ||
success: false, | ||
errorMessage: | ||
"The on-chain referendum has rejected the RFC, however I was not able to merge the PR automatically.", |
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.
you mean "close PR" ?
as it's not merging
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.
Thanks, changed to close.
src/process.ts
Outdated
"The on-chain referendum has approved the RFC, however I was not able to merge the PR automatically.", | ||
}; | ||
} | ||
} |
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.
else?
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.
The previous if
has a return
, so the else
is not needed.
However I added it anyways.
src/process.ts
Outdated
owner: githubActions.context.repo.owner, | ||
repo: githubActions.context.repo.repo, | ||
pull_number: githubActions.context.issue.number, | ||
merge_method: "squash", | ||
state: "closed", |
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.
Does it close the Pull request? Is it desired to close, or might they have additional work and another round of vote?
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.
Yes, we close it because in this situation it's not just a missing approval, in this situation we have an explicit RFC_REJECT
referendum which passed. I pushed a commit trying to make it a little more explicit.
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.
Here is some more explanation:
rfc-action/src/find-referendum-state.ts
Lines 9 to 15 in 708c628
/** | |
* @returns Find the state of a referendum concerning this RFC. | |
* The returned RFC referendum state can be one of: | |
* - approved (and executed) - meaning the referendum approving this RFC has passed and has been executed, | |
* - rejected (and executed) - meaning the referendum rejecting this RFC has passed and has been executed, | |
* - null, meaning that the referendum in a proper state with a proper remark has not been found. It's possible there is a referendum approving or rejecting this RFC but has not passed and not been executed yet. | |
*/ |
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 would recommend to link to the logs.
You can do something along this lines:
const logs = `${context.serverUrl}/${repo.owner}/${repo.repo}/actions/runs/${context.runId}`;
await comment(`There was a problem. Check the [logs here](${logs})`);
It makes it easier to debug.
Great suggestion, already auto-merged so will do in #21 |
Instead of completely failing when lacking the merge/close permissions, it will print an appropriate comment in the PR.