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.

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

Merged
merged 8 commits into from Jun 23, 2022
Merged

Conversation

jtbandes
Copy link
Contributor

@jtbandes jtbandes commented Jun 23, 2022

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

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 | |

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.

Copy link
Collaborator

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

Copy link
Contributor Author

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?

Copy link
Collaborator

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.

Copy link
Contributor Author

@jtbandes jtbandes Jun 23, 2022

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"
}

Copy link
Collaborator

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?

Copy link
Contributor Author

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.

Copy link
Collaborator

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

Copy link
Contributor Author

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!

@jtbandes jtbandes changed the title Add close-as-not-planned option Add close-issue-reason option Jun 23, 2022
Copy link
Collaborator

@luketomlinson luketomlinson left a 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! 🥇

@luketomlinson luketomlinson merged commit 06d2a39 into actions:main Jun 23, 2022
5 checks passed
@jtbandes jtbandes deleted the jacob/not-planned branch June 23, 2022 21:49
@jtbandes
Copy link
Contributor Author

Great, thanks for reviewing/merging! Are you planning to make a new release soon as well?

jtbandes added a commit to RobotWebTools/rosbridge_suite that referenced this pull request Aug 1, 2022
**User-Facing Changes**
None

**Description**
Uses actions/stale#764 to close issues as not planned instead of completed
jtbandes added a commit to foxglove/studio that referenced this pull request Aug 2, 2022
**User-Facing Changes**
None

**Description**
Uses actions/stale#764 to close issues as not planned instead of completed
jtbandes added a commit to RobotWebTools/rosbridge_suite that referenced this pull request Aug 2, 2022
**User-Facing Changes**
None

**Description**
Uses actions/stale#764 to close issues as not planned instead of completed
jihoonl pushed a commit to floatic-unicorn/rosbridge_suite that referenced this pull request Oct 6, 2022
**User-Facing Changes**
None

**Description**
Uses actions/stale#764 to close issues as not planned instead of completed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support the "not planned" state reason (when available)
3 participants