-
Notifications
You must be signed in to change notification settings - Fork 353
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
Add close-issue-reason option #764
Conversation
README.md
Outdated
@@ -80,6 +80,7 @@ Every argument is optional. | |||
| [ignore-updates](#ignore-updates) | Any update (update/comment) can reset the stale idle time on the issues/PRs | `false` | | |||
| [ignore-issue-updates](#ignore-issue-updates) | Override [ignore-updates](#ignore-updates) for issues only | | | |||
| [ignore-pr-updates](#ignore-pr-updates) | Override [ignore-updates](#ignore-updates) for PRs only | | | |||
| [close-as-not-planned](#close-as-not-planned) | Use the "not planned" close reason for issues | | |
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.
IMO because this only affects issues (PR closed state is either unmerged or merged), and because of the existence of options like close-issue-message
, close-issue-label
, etc. This option should be close-issue-<something>
.
Also, right now it's a boolean (either not planned or completed), but that can change if GitHub ever introduces a 3rd closed state. That's why I suggested in #744 (comment) to have a close-issue-reason
option that takes a string.
But don't make any changes just for me 🙂
Let's wait and see what the maintainers think.
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.
yeah I think I like the close-issue-reason
improvement, which makes us a little more resilient/future proof to any new states
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 you want to validate against a known set of values, or just pass it through to the API verbatim?
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 think validating against known values seems like a good idea. I haven't explored what happens if you were to pass state_reason: foo
, so it's possible the API throws out bad values, but it's very possible it might also throw a 400.
So I think throwing out any values that aren't in the list ["closed", "not_planned"]
makes sense.
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.
If I pass an invalid reason, including an empty string, I get 422 Unprocessable Entity:
{
"message": "Please specify a valid state reason.",
"documentation_url": "https://docs.github.com/rest/reference/issues#update-an-issue"
}
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.
ah ok, I think in that case, it makes sense to check if it's one of the 2 valid values. If not, just fallback to the default. What do you think?
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.
Sounds good, it just means that if GitHub introduces a new value in the future, we'll have to push another update to this action.
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.
yep, understood. IMHO that's preferable to potentially breaking the action in a potentially confusing way
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.
@luketomlinson changes made, please take another look when you get a chance. Thanks!
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.
LGTM. Thanks for doing this @jtbandes! 🥇
Great, thanks for reviewing/merging! Are you planning to make a new release soon as well? |
**User-Facing Changes** None **Description** Uses actions/stale#764 to close issues as not planned instead of completed
**User-Facing Changes** None **Description** Uses actions/stale#764 to close issues as not planned instead of completed
**User-Facing Changes** None **Description** Uses actions/stale#764 to close issues as not planned instead of completed
Changes
Adds an option
close-issue-reason
, which can be used to close stale issues as "not planned" (a relatively new GitHub feature).I tested the
state_reason: "not_planned"
option by making manual calls to the REST API (unfortunately it is not currently listed in the REST API documentation).Context
Resolves #744